Moving ownership of DRP blacklist skipping to Previews

This CL moves the state of whether DRP can skip the long term blacklist
checks for Previews from DRPConfig to PreviewsDecider. This happens
through some thread Posts, and will eventually move to one thread
PostTask instead of 2 when Previews is on the UI thread.

Bug: 842233
Change-Id: Id8129338eeb1b2401f8f70ab2d02aebcbbd5c0e1
Reviewed-on: https://chromium-review.googlesource.com/c/1252783
Commit-Queue: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596331}

chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc

@@ -700,8 +700,15 @@ ChromeResourceDispatcherHostDelegate::DetermineEnabledPreviews(
   if (data_reduction_proxy_io_data && previews_decider_impl) {
     previews::PreviewsUserData::Create(url_request,
                                        previews_decider_impl->GeneratePageId());
-    if (data_reduction_proxy_io_data->ShouldAcceptServerPreview(
-            *url_request, previews_decider_impl)) {
+    if (url_request->url().SchemeIsHTTPOrHTTPS() &&
+        previews_decider_impl->ShouldAllowPreviewAtECT(
+            *url_request, previews::PreviewsType::LITE_PAGE,
+            net::EFFECTIVE_CONNECTION_TYPE_4G, std::vector<std::string>(),
+            true) &&
+        previews_decider_impl->ShouldAllowPreviewAtECT(
+            *url_request, previews::PreviewsType::LOFI,
+            net::EFFECTIVE_CONNECTION_TYPE_4G, std::vector<std::string>(),
+            true)) {
       previews_state |= content::SERVER_LOFI_ON;
       previews_state |= content::SERVER_LITE_PAGE_ON;
     }

chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc

@@ -17,6 +17,8 @@
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
+#include "chrome/browser/previews/previews_service.h"
+#include "chrome/browser/previews/previews_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/pref_names.h"
@@ -30,6 +32,7 @@
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
+#include "components/previews/content/previews_ui_service.h"
 #include "components/proxy_config/proxy_config_pref_names.h"
 #include "components/proxy_config/proxy_prefs.h"
 #include "content/public/browser/browser_thread.h"
@@ -175,8 +178,8 @@ DataReductionProxyChromeSettings::MigrateDataReductionProxyOffProxyPrefsHelper(
 
 DataReductionProxyChromeSettings::DataReductionProxyChromeSettings()
     : data_reduction_proxy::DataReductionProxySettings(),
-      data_reduction_proxy_enabled_pref_name_(prefs::kDataSaverEnabled) {
-}
+      data_reduction_proxy_enabled_pref_name_(prefs::kDataSaverEnabled),
+      profile_(nullptr) {}
 
 DataReductionProxyChromeSettings::~DataReductionProxyChromeSettings() {
 }
@@ -192,10 +195,12 @@ void DataReductionProxyChromeSettings::InitDataReductionProxySettings(
     data_reduction_proxy::DataReductionProxyIOData* io_data,
     PrefService* profile_prefs,
     net::URLRequestContextGetter* request_context_getter,
+    Profile* profile,
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     std::unique_ptr<data_reduction_proxy::DataStore> store,
     const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner,
     const scoped_refptr<base::SequencedTaskRunner>& db_task_runner) {
+  profile_ = profile;
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 #if defined(OS_ANDROID)
   // On mobile we write Data Reduction Proxy prefs directly to the pref service.
@@ -231,6 +236,18 @@ void DataReductionProxyChromeSettings::InitDataReductionProxySettings(
   MigrateDataReductionProxyOffProxyPrefs(profile_prefs);
 }
 
+void DataReductionProxyChromeSettings::SetIgnoreLongTermBlackListRules(
+    bool ignore_long_term_black_list_rules) {
+  // |previews_service| is null if |profile_| is off the record.
+  PreviewsService* previews_service =
+      PreviewsServiceFactory::GetForProfile(profile_);
+  if (previews_service && previews_service->previews_ui_service()) {
+    previews_service->previews_ui_service()
+        ->SetIgnoreLongTermBlackListForServerPreviews(
+            ignore_long_term_black_list_rules);
+  }
+}
+
 // static
 data_reduction_proxy::Client DataReductionProxyChromeSettings::GetClient() {
 #if defined(OS_ANDROID)

chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h

@@ -14,6 +14,7 @@
 #include "components/keyed_service/core/keyed_service.h"
 
 class PrefService;
+class Profile;
 
 namespace base {
 class SequencedTaskRunner;
@@ -72,6 +73,7 @@ class DataReductionProxyChromeSettings
       data_reduction_proxy::DataReductionProxyIOData* io_data,
       PrefService* profile_prefs,
       net::URLRequestContextGetter* request_context_getter,
+      Profile* profile,
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       std::unique_ptr<data_reduction_proxy::DataStore> store,
       const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner,
@@ -90,6 +92,9 @@ class DataReductionProxyChromeSettings
     data_reduction_proxy_enabled_pref_name_ = pref_name;
   }
 
+  void SetIgnoreLongTermBlackListRules(
+      bool ignore_long_term_black_list_rules) override;
+
  private:
   // Helper method for migrating the Data Reduction Proxy away from using the
   // proxy pref. Returns the ProxyPrefMigrationResult value indicating the
@@ -99,6 +104,9 @@ class DataReductionProxyChromeSettings
 
   std::string data_reduction_proxy_enabled_pref_name_;
 
+  // Null before InitDataReductionProxySettings is called.
+  Profile* profile_;
+
   DISALLOW_COPY_AND_ASSIGN(DataReductionProxyChromeSettings);
 };
 

chrome/browser/previews/previews_infobar_delegate_unittest.cc

@@ -35,6 +35,7 @@
 #include "chrome/grit/generated_resources.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_browser_process.h"
+#include "chrome/test/base/testing_profile.h"
 #include "components/blacklist/opt_out_blacklist/opt_out_blacklist_data.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h"
@@ -185,7 +186,7 @@ class PreviewsInfoBarDelegateUnitTest
             drp_test_context_->GetDataReductionProxyEnabledPrefName());
     data_reduction_proxy_settings->InitDataReductionProxySettings(
         drp_test_context_->io_data(), drp_test_context_->pref_service(),
-        drp_test_context_->request_context_getter(),
+        drp_test_context_->request_context_getter(), profile(),
         base::MakeRefCounted<network::TestSharedURLLoaderFactory>(),
         base::WrapUnique(new data_reduction_proxy::DataStore()),
         base::ThreadTaskRunnerHandle::Get(),

chrome/browser/previews/previews_ui_tab_helper_unittest.cc

@@ -19,6 +19,7 @@
 #include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.h"
 #include "chrome/browser/previews/previews_ui_tab_helper.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "chrome/test/base/testing_profile.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h"
@@ -80,7 +81,7 @@ class PreviewsUITabHelperUnitTest : public ChromeRenderViewHostTestHarness {
             drp_test_context_->GetDataReductionProxyEnabledPrefName());
     data_reduction_proxy_settings->InitDataReductionProxySettings(
         drp_test_context_->io_data(), drp_test_context_->pref_service(),
-        drp_test_context_->request_context_getter(),
+        drp_test_context_->request_context_getter(), profile(),
         base::MakeRefCounted<network::TestSharedURLLoaderFactory>(),
         base::WrapUnique(new data_reduction_proxy::DataStore()),
         base::ThreadTaskRunnerHandle::Get(),

chrome/browser/profiles/profile_impl_io_data.cc

@@ -244,6 +244,7 @@ ProfileImplIOData::Handle::CreateMainRequestContextGetter(
           base::FeatureList::IsEnabled(network::features::kNetworkService)
               ? nullptr
               : main_request_context_getter_.get(),
+          profile_,
           content::BrowserContext::GetDefaultStoragePartition(profile_)
               ->GetURLLoaderFactoryForBrowserProcess(),
           std::move(store),

chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc

@@ -439,7 +439,7 @@ class RenderViewContextMenuPrefsTest : public ChromeRenderViewHostTestHarness {
         drp_test_context_->GetDataReductionProxyEnabledPrefName());
     settings->InitDataReductionProxySettings(
         drp_test_context_->io_data(), drp_test_context_->pref_service(),
-        drp_test_context_->request_context_getter(),
+        drp_test_context_->request_context_getter(), profile(),
         base::MakeRefCounted<network::TestSharedURLLoaderFactory>(),
         std::make_unique<data_reduction_proxy::DataStore>(),
         base::ThreadTaskRunnerHandle::Get(),

components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc

@@ -71,17 +71,6 @@ base::LazySequencedTaskRunner g_get_network_id_task_runner =
                          base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN));
 #endif
 
-// Values of the UMA DataReductionProxy.Protocol.NotAcceptingTransform histogram
-// defined in metrics/histograms/histograms.xml. This enum must remain
-// synchronized with DataReductionProxyProtocolNotAcceptingTransformReason in
-// tools/metrics/histograms/enums.xml.
-enum NotAcceptingTransformReason {
-  NOT_ACCEPTING_TRANSFORM_DISABLED = 0,
-  NOT_ACCEPTING_TRANSFORM_BLACKLISTED = 1,
-  NOT_ACCEPTING_TRANSFORM_CELLULAR_ONLY = 2,
-  NOT_ACCEPTING_TRANSFORM_REASON_BOUNDARY
-};
-
 // Values of the UMA DataReductionProxy.NetworkChangeEvents histograms.
 // This enum must remain synchronized with the enum of the same
 // name in metrics/histograms/histograms.xml.
@@ -209,7 +198,6 @@ DataReductionProxyConfig::DataReductionProxyConfig(
       configurator_(configurator),
       event_creator_(event_creator),
       connection_type_(network::mojom::ConnectionType::CONNECTION_UNKNOWN),
-      ignore_long_term_black_list_rules_(false),
       network_properties_manager_(nullptr),
       weak_factory_(this) {
   DCHECK(io_task_runner_);
@@ -792,46 +780,6 @@ bool DataReductionProxyConfig::enabled_by_user_and_reachable() const {
   return enabled_by_user_ && !unreachable_;
 }
 
-bool DataReductionProxyConfig::IsBlackListedOrDisabled(
-    const net::URLRequest& request,
-    const previews::PreviewsDecider& previews_decider,
-    previews::PreviewsType previews_type) const {
-  // Make sure request is not locally blacklisted.
-  // Pass in net::EFFECTIVE_CONNECTION_TYPE_4G as the threshold since we
-  // just want to check blacklisting here.
-  // TODO(crbug.com/720102): Consider new method to just check blacklist.
-  return !previews_decider.ShouldAllowPreviewAtECT(
-      request, previews_type, net::EFFECTIVE_CONNECTION_TYPE_4G,
-      std::vector<std::string>(), ignore_long_term_black_list_rules_);
-}
-
-bool DataReductionProxyConfig::ShouldAcceptServerPreview(
-    const net::URLRequest& request,
-    const previews::PreviewsDecider& previews_decider) const {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK((request.load_flags() & net::LOAD_MAIN_FRAME_DEPRECATED) != 0);
-  DCHECK(request.url().SchemeIsHTTPOrHTTPS());
-
-  if (!previews::params::ArePreviewsAllowed() ||
-      !base::FeatureList::IsEnabled(
-          features::kDataReductionProxyDecidesTransform)) {
-    return false;
-  }
-
-  if (IsBlackListedOrDisabled(request, previews_decider,
-                              previews::PreviewsType::LITE_PAGE) ||
-      IsBlackListedOrDisabled(request, previews_decider,
-                              previews::PreviewsType::LOFI)) {
-    UMA_HISTOGRAM_ENUMERATION(
-        "DataReductionProxy.Protocol.NotAcceptingTransform",
-        NOT_ACCEPTING_TRANSFORM_BLACKLISTED,
-        NOT_ACCEPTING_TRANSFORM_REASON_BOUNDARY);
-    return false;
-  }
-
-  return true;
-}
-
 base::TimeTicks DataReductionProxyConfig::GetTicksNow() const {
   DCHECK(thread_checker_.CalledOnValidThread());
   return base::TimeTicks::Now();
@@ -888,15 +836,4 @@ void DataReductionProxyConfig::EnableGetNetworkIdAsynchronously() {
 }
 #endif  // defined(OS_CHROMEOS)
 
-void DataReductionProxyConfig::SetIgnoreLongTermBlackListRules(
-    bool ignore_long_term_black_list_rules) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  ignore_long_term_black_list_rules_ = ignore_long_term_black_list_rules;
-}
-
-bool DataReductionProxyConfig::IgnoreBlackListLongTermRulesForTesting() const {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  return ignore_long_term_black_list_rules_;
-}
-
 }  // namespace data_reduction_proxy

components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h

@@ -43,10 +43,6 @@ class URLRequestContextGetter;
 class URLRequestStatus;
 }  // namespace net
 
-namespace previews {
-class PreviewsDecider;
-}
-
 namespace data_reduction_proxy {
 
 class DataReductionProxyConfigValues;
@@ -159,15 +155,6 @@ class DataReductionProxyConfig
   virtual bool ContainsDataReductionProxy(
       const net::ProxyConfig::ProxyRules& proxy_rules) const;
 
-  // Returns whether the client should report to the data reduction proxy that
-  // it is willing to accept server previews for |request|.
-  // |previews_decider| is used to check if |request| is locally blacklisted.
-  // Should only be used if the kDataReductionProxyDecidesTransform feature is
-  // enabled.
-  bool ShouldAcceptServerPreview(
-      const net::URLRequest& request,
-      const previews::PreviewsDecider& previews_decider) const;
-
   // Returns true if the data saver has been enabled by the user, and the data
   // saver proxy is reachable.
   bool enabled_by_user_and_reachable() const;
@@ -202,19 +189,12 @@ class DataReductionProxyConfig
   // TODO(https://crbug.com/821607): Remove after the bug is resolved.
   void EnableGetNetworkIdAsynchronously();
 #endif  // defined(OS_CHROMEOS)
-
-  // When triggering previews, prevent long term black list rules.
-  void SetIgnoreLongTermBlackListRules(bool ignore_long_term_black_list_rules);
-
   // Called when there is a change in the HTTP RTT estimate.
   void OnRTTOrThroughputEstimatesComputed(base::TimeDelta http_rtt);
 
   // Returns the current HTTP RTT estimate.
   base::Optional<base::TimeDelta> GetHttpRttEstimate() const;
 
-  // Returns the value set in SetIgnoreLongTermBlackListRules.
-  bool IgnoreBlackListLongTermRulesForTesting() const;
-
  protected:
   virtual base::TimeTicks GetTicksNow() const;
 
@@ -312,11 +292,6 @@ class DataReductionProxyConfig
                           bool is_https,
                           base::TimeDelta* min_retry_delay) const;
 
-  // Returns whether the request is blacklisted (or if Lo-Fi is disabled).
-  bool IsBlackListedOrDisabled(
-      const net::URLRequest& request,
-      const previews::PreviewsDecider& previews_decider,
-      previews::PreviewsType previews_type) const;
 
   // Checks if the current network has captive portal, and handles the result.
   // If the captive portal probe was blocked on the current network, disables
@@ -378,9 +353,6 @@ class DataReductionProxyConfig
   bool warmup_url_fetch_in_flight_secure_proxy_;
   bool warmup_url_fetch_in_flight_core_proxy_;
 
-  // When triggerring previews, prevent long term black list rules.
-  bool ignore_long_term_black_list_rules_;
-
   // Should be accessed only on the IO thread. Guaranteed to be non-null during
   // the lifetime of |this| if accessed on the IO thread.
   NetworkPropertiesManager* network_properties_manager_;

components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc

@@ -606,7 +606,7 @@ bool DataReductionProxyConfigServiceClient::ParseAndApplyProxyConfig(
   if (!config.has_proxy_config())
     return false;
 
-  config_->SetIgnoreLongTermBlackListRules(
+  io_data_->SetIgnoreLongTermBlackListRules(
       config.ignore_long_term_black_list_rules());
 
   // An empty proxy config is OK, and allows the server to effectively turn off

components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc

@@ -348,6 +348,10 @@ class DataReductionProxyConfigServiceClientTest : public testing::Test {
     return test_context_->io_data()->pingback_reporting_fraction();
   }
 
+  bool ignore_blacklist() const {
+    return test_context_->io_data()->ignore_blacklist();
+  }
+
   void RunUntilIdle() {
     test_context_->RunUntilIdle();
   }
@@ -1358,15 +1362,15 @@ TEST_F(DataReductionProxyConfigServiceClientTest,
   config_client()->ApplySerializedConfig(
       half_reporting_fraction_encoded_config());
   EXPECT_EQ(0.5f, pingback_reporting_fraction());
-  EXPECT_FALSE(config()->IgnoreBlackListLongTermRulesForTesting());
+  EXPECT_FALSE(ignore_blacklist());
 }
 
 TEST_F(DataReductionProxyConfigServiceClientTest,
        ApplySerializedConfigIgnoreBlackList) {
   Init(true);
 
   config_client()->ApplySerializedConfig(ignore_black_list_encoded_config());
-  EXPECT_TRUE(config()->IgnoreBlackListLongTermRulesForTesting());
+  EXPECT_TRUE(ignore_blacklist());
 }
 
 TEST_F(DataReductionProxyConfigServiceClientTest, EmptyConfigDisablesDRP) {