[Previews] Adding DataSaver check for using HintsFetcher.

Checks for DataSaver enabled users before allowing HintsFetching to
occur. PreviewsOptimizationGuide is provided a PrefService and access
to the DataReductionProxy to determine if DataSaver is enabled.

Bug: 932707
Change-Id: I13fbbe7ca2669ce6b0b97a7199a40bf179c281cd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1604211
Commit-Queue: Michael Crouse <mcrouse@chromium.org>
Auto-Submit: Michael Crouse <mcrouse@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659974}

chrome/browser/previews/previews_service.cc

@@ -112,6 +112,7 @@ PreviewsService::PreviewsService(content::BrowserContext* browser_context)
           std::make_unique<PreviewsLitePageDecider>(browser_context)),
       previews_offline_helper_(
           std::make_unique<PreviewsOfflineHelper>(browser_context)),
+      browser_context_(browser_context),
       previews_url_loader_factory_(
           content::BrowserContext::GetDefaultStoragePartition(
               Profile::FromBrowserContext(browser_context))
@@ -147,8 +148,10 @@ void PreviewsService::Initialize(
       optimization_guide_service
           ? std::make_unique<previews::PreviewsOptimizationGuide>(
                 optimization_guide_service, ui_task_runner,
-                background_task_runner, profile_path, database_provider,
-                previews_top_host_provider_.get(), previews_url_loader_factory_)
+                background_task_runner, profile_path,
+                Profile::FromBrowserContext(browser_context_)->GetPrefs(),
+                database_provider, previews_top_host_provider_.get(),
+                previews_url_loader_factory_)
           : nullptr,
       base::Bind(&IsPreviewsTypeEnabled),
       std::make_unique<previews::PreviewsLogger>(), GetAllowedPreviews(),

chrome/browser/previews/previews_service.h

@@ -107,6 +107,9 @@ class PreviewsService : public KeyedService {
   // The offline previews helper.
   std::unique_ptr<PreviewsOfflineHelper> previews_offline_helper_;
 
+  // Guaranteed to outlive |this|.
+  content::BrowserContext* browser_context_;
+
   // URL Factory for the Previews Optimization Guide's Hints Fetcher.
   scoped_refptr<network::SharedURLLoaderFactory> previews_url_loader_factory_;
 

components/previews/content/BUILD.gn

@@ -30,8 +30,10 @@ static_library("content") {
   deps = [
     "//base",
     "//components/blacklist/opt_out_blacklist",
+    "//components/data_reduction_proxy/core/browser",
     "//components/optimization_guide",
     "//components/optimization_guide/proto:optimization_guide_proto",
+    "//components/prefs",
     "//components/previews/content/proto:hint_cache_proto",
     "//components/previews/core",
     "//components/url_matcher",
@@ -63,11 +65,14 @@ source_set("unit_tests") {
     ":content",
     "//base",
     "//components/blacklist/opt_out_blacklist",
+    "//components/data_reduction_proxy/core/browser",
+    "//components/data_reduction_proxy/core/common",
     "//components/keyed_service/core:test_support",
     "//components/leveldb_proto:test_support",
     "//components/leveldb_proto/content:factory",
     "//components/optimization_guide",
     "//components/optimization_guide/proto:optimization_guide_proto",
+    "//components/prefs:test_support",
     "//components/previews/content/proto:hint_cache_proto",
     "//components/previews/core",
     "//components/variations",

components/previews/content/DEPS

 include_rules = [
   "+components/blacklist/opt_out_blacklist",
+  "+components/data_reduction_proxy/core/browser",
+  "+components/data_reduction_proxy/core/common",
   "+components/leveldb_proto",
   "+components/keyed_service",
+  "+components/prefs",
   "+components/optimization_guide",
   "+components/previews/core",
   "+components/url_matcher",

components/previews/content/previews_decider_impl_unittest.cc

@@ -36,9 +36,11 @@
 #include "components/blacklist/opt_out_blacklist/opt_out_blacklist_delegate.h"
 #include "components/blacklist/opt_out_blacklist/opt_out_blacklist_item.h"
 #include "components/blacklist/opt_out_blacklist/opt_out_store.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h"
 #include "components/keyed_service/core/test_simple_factory_key.h"
 #include "components/leveldb_proto/content/proto_database_provider_factory.h"
 #include "components/optimization_guide/optimization_guide_service.h"
+#include "components/prefs/testing_pref_service.h"
 #include "components/previews/content/hint_cache_store.h"
 #include "components/previews/content/previews_hints.h"
 #include "components/previews/content/previews_top_host_provider.h"
@@ -157,6 +159,7 @@ class TestPreviewsOptimizationGuide : public PreviewsOptimizationGuide {
       const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner,
       const scoped_refptr<base::SequencedTaskRunner>& background_task_runner,
       const base::FilePath& test_path,
+      PrefService* pref_service,
       leveldb_proto::ProtoDatabaseProvider* database_provider,
       PreviewsTopHostProvider* previews_top_host_provider,
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
@@ -164,6 +167,7 @@ class TestPreviewsOptimizationGuide : public PreviewsOptimizationGuide {
                                   ui_task_runner,
                                   background_task_runner,
                                   test_path,
+                                  pref_service,
                                   database_provider,
                                   previews_top_host_provider,
                                   url_loader_factory) {}
@@ -389,7 +393,12 @@ class PreviewsDeciderImplTest : public ProtoDatabaseProviderTestBase {
     variations::testing::ClearAllVariationParams();
   }
 
-  void SetUp() override { ProtoDatabaseProviderTestBase::SetUp(); }
+  void SetUp() override {
+    // Enable DataSaver for checks with PreviewsOptimizationGuide.
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+        data_reduction_proxy::switches::kEnableDataReductionProxy);
+    ProtoDatabaseProviderTestBase::SetUp();
+  }
 
   void TearDown() override {
     ProtoDatabaseProviderTestBase::TearDown();
@@ -408,6 +417,7 @@ class PreviewsDeciderImplTest : public ProtoDatabaseProviderTestBase {
     std::unique_ptr<TestPreviewsDeciderImpl> previews_decider_impl =
         std::make_unique<TestPreviewsDeciderImpl>(&clock_);
     previews_decider_impl_ = previews_decider_impl.get();
+    pref_service_ = std::make_unique<TestingPrefServiceSimple>();
     ui_service_.reset(new TestPreviewsUIService(
         std::move(previews_decider_impl), std::make_unique<TestOptOutStore>(),
         std::make_unique<TestPreviewsOptimizationGuide>(
@@ -415,8 +425,8 @@ class PreviewsDeciderImplTest : public ProtoDatabaseProviderTestBase {
             scoped_task_environment_.GetMainThreadTaskRunner(),
             base::CreateSequencedTaskRunnerWithTraits(
                 {base::MayBlock(), base::TaskPriority::BEST_EFFORT}),
-            temp_dir_.GetPath(), db_provider_, &previews_top_host_provider_,
-            url_loader_factory_),
+            temp_dir_.GetPath(), pref_service_.get(), db_provider_,
+            &previews_top_host_provider_, url_loader_factory_),
         base::BindRepeating(&IsPreviewFieldTrialEnabled),
         std::make_unique<PreviewsLogger>(), std::move(allowed_types),
         &network_quality_tracker_));
@@ -460,6 +470,7 @@ class PreviewsDeciderImplTest : public ProtoDatabaseProviderTestBase {
   TestPreviewsTopHostProvider previews_top_host_provider_;
   std::unique_ptr<TestPreviewsUIService> ui_service_;
   network::TestNetworkQualityTracker network_quality_tracker_;
+  std::unique_ptr<TestingPrefServiceSimple> pref_service_;
   scoped_refptr<network::TestSharedURLLoaderFactory> url_loader_factory_;
 };
 

components/previews/content/previews_optimization_guide.cc

@@ -13,9 +13,11 @@
 #include "base/task/post_task.h"
 #include "base/task_runner_util.h"
 #include "base/time/default_clock.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h"
 #include "components/optimization_guide/hints_component_info.h"
 #include "components/optimization_guide/optimization_guide_service.h"
 #include "components/optimization_guide/proto/hints.pb.h"
+#include "components/prefs/pref_service.h"
 #include "components/previews/content/hint_cache_store.h"
 #include "components/previews/content/hints_fetcher.h"
 #include "components/previews/content/previews_hints.h"
@@ -128,6 +130,7 @@ PreviewsOptimizationGuide::PreviewsOptimizationGuide(
     const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner,
     const scoped_refptr<base::SequencedTaskRunner>& background_task_runner,
     const base::FilePath& profile_path,
+    PrefService* pref_service,
     leveldb_proto::ProtoDatabaseProvider* database_provider,
     PreviewsTopHostProvider* previews_top_host_provider,
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
@@ -140,6 +143,7 @@ PreviewsOptimizationGuide::PreviewsOptimizationGuide(
                                            background_task_runner_))),
       previews_top_host_provider_(previews_top_host_provider),
       time_clock_(base::DefaultClock::GetInstance()),
+      pref_service_(pref_service),
       url_loader_factory_(url_loader_factory),
       ui_weak_ptr_factory_(this) {
   DCHECK(optimization_guide_service_);
@@ -372,6 +376,7 @@ void PreviewsOptimizationGuide::UpdateHints(
 void PreviewsOptimizationGuide::OnHintsUpdated(
     base::OnceClosure update_closure) {
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
+  DCHECK(pref_service_);
   if (!update_closure.is_null())
     std::move(update_closure).Run();
 
@@ -385,25 +390,34 @@ void PreviewsOptimizationGuide::OnHintsUpdated(
   // flag |kOptimizationHintsFetching|, fetch hints from the remote Optimization
   // Guide Service.
   //
-  // TODO(mcrouse): Add a check for user specific state in addition to the
-  // feature state: (1) Data saver should be enabled (2) Check if Infobar
-  // notification needs to be shown to the user.
-
-  if (previews::params::IsHintsFetchingEnabled()) {
-    if (ParseHintsFetchOverrideFromCommandLine()) {
-      // Skip the fetch scheduling logic and perform a hints fetch immediately
-      // after initialization.
-      last_fetch_attempt_ = time_clock_->Now();
-      FetchHints();
-    } else {
-      ScheduleHintsFetch();
-    }
+  // TODO(mcrouse): Add a check if Infobar notification needs to be shown to the
+  // user.
+  if (!data_reduction_proxy::DataReductionProxySettings::
+          IsDataSaverEnabledByUser(pref_service_)) {
+    return;
+  }
+
+  if (!previews::params::IsHintsFetchingEnabled())
+    return;
+
+  if (ParseHintsFetchOverrideFromCommandLine()) {
+    // Skip the fetch scheduling logic and perform a hints fetch immediately
+    // after initialization.
+    last_fetch_attempt_ = time_clock_->Now();
+    FetchHints();
+  } else {
+    ScheduleHintsFetch();
   }
 }
 
 void PreviewsOptimizationGuide::ScheduleHintsFetch() {
   DCHECK(!hints_fetch_timer_.IsRunning());
+  DCHECK(pref_service_);
 
+  if (!data_reduction_proxy::DataReductionProxySettings::
+          IsDataSaverEnabledByUser(pref_service_)) {
+    return;
+  }
   const base::TimeDelta time_until_update_time =
       hint_cache_->FetchedHintsUpdateTime() - time_clock_->Now();
   const base::TimeDelta time_until_retry =
@@ -411,15 +425,15 @@ void PreviewsOptimizationGuide::ScheduleHintsFetch() {
   base::TimeDelta fetcher_delay;
   if (time_until_update_time <= base::TimeDelta() &&
       time_until_retry <= base::TimeDelta()) {
-    // Fetched hints in the store should be updated and an attempt has not been
-    // made in last |kFetchRetryDelay|.
+    // Fetched hints in the store should be updated and an attempt has not
+    // been made in last |kFetchRetryDelay|.
     last_fetch_attempt_ = time_clock_->Now();
     hints_fetch_timer_.Start(FROM_HERE, RandomFetchDelay(), this,
                              &PreviewsOptimizationGuide::FetchHints);
   } else {
     if (time_until_update_time >= base::TimeDelta()) {
-      // If the fetched hints in the store are still up-to-date, set a timer for
-      // when the hints need to be updated.
+      // If the fetched hints in the store are still up-to-date, set a timer
+      // for when the hints need to be updated.
       fetcher_delay = time_until_update_time;
     } else {
       // Otherwise, hints need to be updated but an attempt was made in last

components/previews/content/previews_optimization_guide.h

@@ -23,6 +23,8 @@
 #include "components/previews/core/previews_experiments.h"
 #include "url/gurl.h"
 
+class PrefService;
+
 namespace base {
 class FilePath;
 }  // namespace base
@@ -56,6 +58,7 @@ class PreviewsOptimizationGuide
       const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner,
       const scoped_refptr<base::SequencedTaskRunner>& background_task_runner,
       const base::FilePath& profile_path,
+      PrefService* pref_service,
       leveldb_proto::ProtoDatabaseProvider* database_provider,
       PreviewsTopHostProvider* previews_top_host_provider,
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
@@ -128,7 +131,6 @@ class PreviewsOptimizationGuide
   // should be fetched and schedules the |hints_fetch_timer_| to fire based on:
   // 1. The update time for the fetched hints in the store and
   // 2. The last time a fetch attempt was made, |last_fetch_attempt_|.
-  // TODONOW(mcrouse) : confirm is this is ok or not.
   void ScheduleHintsFetch();
 
  protected:
@@ -202,6 +204,9 @@ class PreviewsOptimizationGuide
 
   base::Time last_fetch_attempt_;
 
+  // A reference to the PrefService for this profile. Not owned.
+  PrefService* pref_service_ = nullptr;
+
   // Used for fetching Hints by the Hints Fetcher.
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 

components/previews/content/previews_optimization_guide_unittest.cc

@@ -22,11 +22,15 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/test/simple_test_clock.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h"
 #include "components/keyed_service/core/test_simple_factory_key.h"
 #include "components/leveldb_proto/content/proto_database_provider_factory.h"
 #include "components/optimization_guide/hints_component_info.h"
 #include "components/optimization_guide/optimization_guide_service.h"
 #include "components/optimization_guide/proto/hints.pb.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/testing_pref_service.h"
 #include "components/previews/content/hints_fetcher.h"
 #include "components/previews/content/previews_hints.h"
 #include "components/previews/content/previews_top_host_provider.h"
@@ -163,6 +167,7 @@ class TestPreviewsOptimizationGuide : public PreviewsOptimizationGuide {
       const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner,
       const scoped_refptr<base::SequencedTaskRunner>& background_task_runner,
       const base::FilePath& profile_path,
+      PrefService* pref_service,
       leveldb_proto::ProtoDatabaseProvider* database_provider,
       PreviewsTopHostProvider* previews_top_host_provider,
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
@@ -170,6 +175,7 @@ class TestPreviewsOptimizationGuide : public PreviewsOptimizationGuide {
                                   ui_task_runner,
                                   background_task_runner,
                                   profile_path,
+                                  pref_service,
                                   database_provider,
                                   previews_top_host_provider,
                                   url_loader_factory) {}
@@ -203,6 +209,7 @@ class PreviewsOptimizationGuideTest : public ProtoDatabaseProviderTestBase {
 
   void SetUp() override {
     ProtoDatabaseProviderTestBase::SetUp();
+    EnableDataSaver();
     CreateServiceAndGuide();
   }
 
@@ -226,6 +233,8 @@ class PreviewsOptimizationGuideTest : public ProtoDatabaseProviderTestBase {
     return url_loader_factory_.get();
   }
 
+  PrefService* pref_service() { return pref_service_.get(); }
+
   TestHintsFetcher* hints_fetcher() {
     return static_cast<TestHintsFetcher*>(guide_->GetHintsFetcherForTesting());
   }
@@ -240,6 +249,16 @@ class PreviewsOptimizationGuideTest : public ProtoDatabaseProviderTestBase {
     RunUntilIdle();
   }
 
+  void EnableDataSaver() {
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+        data_reduction_proxy::switches::kEnableDataReductionProxy);
+  }
+
+  void DisableDataSaver() {
+    base::CommandLine::ForCurrentProcess()->RemoveSwitch(
+        data_reduction_proxy::switches::kEnableDataReductionProxy);
+  }
+
   void CreateServiceAndGuide() {
     if (guide_) {
       ResetGuide();
@@ -254,11 +273,18 @@ class PreviewsOptimizationGuideTest : public ProtoDatabaseProviderTestBase {
     optimization_guide_service_ =
         std::make_unique<TestOptimizationGuideService>(
             scoped_task_environment_.GetMainThreadTaskRunner());
+    pref_service_ = std::make_unique<TestingPrefServiceSimple>();
+
+    // Registry pref for DataSaver with default off.
+    pref_service_->registry()->RegisterBooleanPref(
+        data_reduction_proxy::prefs::kDataSaverEnabled, false);
+
     guide_ = std::make_unique<TestPreviewsOptimizationGuide>(
         optimization_guide_service_.get(),
         scoped_task_environment_.GetMainThreadTaskRunner(),
         scoped_task_environment_.GetMainThreadTaskRunner(), temp_dir(),
-        db_provider_, previews_top_host_provider_.get(), url_loader_factory_);
+        pref_service_.get(), db_provider_, previews_top_host_provider_.get(),
+        url_loader_factory_);
 
     guide_->SetTimeClockForTesting(scoped_task_environment_.GetMockClock());
 
@@ -337,6 +363,10 @@ class PreviewsOptimizationGuideTest : public ProtoDatabaseProviderTestBase {
     }
   }
 
+  // Flag set when the OnLoadOptimizationHints callback runs. This indicates
+  // that MaybeLoadOptimizationHints() has completed its processing.
+  bool requested_hints_loaded_;
+
  private:
   void WriteConfigToFile(const optimization_guide::proto::Configuration& config,
                          const base::FilePath& filePath) {
@@ -356,21 +386,17 @@ class PreviewsOptimizationGuideTest : public ProtoDatabaseProviderTestBase {
   // MaybeLoadOptimizationHints() has completed its processing.
   void OnLoadOptimizationHints();
 
-  // std::unique_ptr<PreviewsOptimizationGuide> guide_;
   std::unique_ptr<TestPreviewsOptimizationGuide> guide_;
   std::unique_ptr<TestOptimizationGuideService> optimization_guide_service_;
   std::unique_ptr<MockPreviewsTopHostProvider> previews_top_host_provider_;
+  std::unique_ptr<TestingPrefServiceSimple> pref_service_;
 
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
   network::TestURLLoaderFactory test_url_loader_factory_;
 
-  // Flag set when the OnLoadOptimizationHints callback runs. This indicates
-  // that MaybeLoadOptimizationHints() has completed its processing.
-  bool requested_hints_loaded_;
 
   GURL loaded_hints_document_gurl_;
   std::vector<std::string> loaded_hints_resource_patterns_;
-  // const base::SimpleTestClock* test_clock_;
 
   DISALLOW_COPY_AND_ASSIGN(PreviewsOptimizationGuideTest);
 };
@@ -1832,4 +1858,41 @@ TEST_F(PreviewsOptimizationGuideTest, HintsFetcherDisabled) {
   InitializeFixedCountResourceLoadingHints();
 }
 
+class PreviewsOptimizationGuideDataSaverOffTest
+    : public PreviewsOptimizationGuideTest {
+ public:
+  PreviewsOptimizationGuideDataSaverOffTest() {}
+
+  ~PreviewsOptimizationGuideDataSaverOffTest() override {}
+
+  void SetUp() override {
+    PreviewsOptimizationGuideTest::SetUp();
+    DisableDataSaver();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PreviewsOptimizationGuideDataSaverOffTest);
+};
+
+TEST_F(PreviewsOptimizationGuideDataSaverOffTest,
+       HintsFetcherEnabledDataSaverDisabled) {
+  base::HistogramTester histogram_tester;
+  base::test::ScopedFeatureList scoped_list;
+  scoped_list.InitAndEnableFeature(features::kOptimizationHintsFetching);
+  std::string opt_guide_url = "https://hintsserver.com";
+
+  guide()->SetHintsFetcherForTesting(
+      BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithHints));
+
+  std::vector<std::string> hosts = {"example1.com", "example2.com"};
+  EXPECT_CALL(*top_host_provider(), GetTopHosts(testing::_)).Times(0);
+
+  // Load hints so that OnHintsUpdated is called. This will force FetchHints to
+  // be triggered if OptimizationHintsFetching is enabled.
+  InitializeFixedCountResourceLoadingHints();
+
+  // Force timer to expire and schedule a hints fetch.
+  MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
+  EXPECT_FALSE(hints_fetcher()->hints_fetched());
+}
 }  // namespace previews