Add hints fetching logic to Optimization Guide Keyed Service

Bug: 969558
Change-Id: Iebeb0e09700f3b4878a6a55507a7ef416f46a9a7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1750092Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686544}

chrome/browser/optimization_guide/optimization_guide_hints_manager.cc

@@ -10,13 +10,16 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros_local.h"
+#include "base/rand_util.h"
 #include "base/sequenced_task_runner.h"
 #include "base/task/post_task.h"
 #include "base/task_runner_util.h"
+#include "base/time/default_clock.h"
 #include "components/optimization_guide/bloom_filter.h"
 #include "components/optimization_guide/hint_cache.h"
 #include "components/optimization_guide/hint_cache_store.h"
 #include "components/optimization_guide/hints_component_util.h"
+#include "components/optimization_guide/hints_fetcher.h"
 #include "components/optimization_guide/hints_processing_util.h"
 #include "components/optimization_guide/optimization_filter.h"
 #include "components/optimization_guide/optimization_guide_features.h"
@@ -27,6 +30,7 @@
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace {
 
@@ -35,6 +39,24 @@ namespace {
 // will have a newer version than it.
 constexpr char kManualConfigComponentVersion[] = "0.0.0";
 
+// Delay between retries on failed fetch and store of hints from the remote
+// Optimization Guide Service.
+constexpr base::TimeDelta kFetchRetryDelay = base::TimeDelta::FromMinutes(15);
+
+// Delay until successfully fetched hints should be updated by requesting from
+// the remote Optimization Guide Service.
+constexpr base::TimeDelta kUpdateFetchedHintsDelay =
+    base::TimeDelta::FromHours(24);
+
+// Provides a random time delta in seconds between |kFetchRandomMinDelay| and
+// |kFetchRandomMaxDelay|.
+base::TimeDelta RandomFetchDelay() {
+  constexpr int kFetchRandomMinDelaySecs = 30;
+  constexpr int kFetchRandomMaxDelaySecs = 60;
+  return base::TimeDelta::FromSeconds(
+      base::RandInt(kFetchRandomMinDelaySecs, kFetchRandomMaxDelaySecs));
+}
+
 void MaybeRunUpdateClosure(base::OnceClosure update_closure) {
   if (update_closure)
     std::move(update_closure).Run();
@@ -79,7 +101,8 @@ OptimizationGuideHintsManager::OptimizationGuideHintsManager(
     const base::FilePath& profile_path,
     PrefService* pref_service,
     leveldb_proto::ProtoDatabaseProvider* database_provider,
-    optimization_guide::TopHostProvider* top_host_provider)
+    optimization_guide::TopHostProvider* top_host_provider,
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
     : optimization_guide_service_(optimization_guide_service),
       background_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
           {base::ThreadPool(), base::MayBlock(),
@@ -91,7 +114,9 @@ OptimizationGuideHintsManager::OptimizationGuideHintsManager(
               profile_path,
               pref_service_,
               background_task_runner_))),
-      top_host_provider_(top_host_provider) {
+      top_host_provider_(top_host_provider),
+      url_loader_factory_(url_loader_factory),
+      clock_(base::DefaultClock::GetInstance()) {
   DCHECK(optimization_guide_service_);
   hint_cache_->Initialize(
       optimization_guide::switches::ShouldPurgeHintCacheStoreOnStartup(),
@@ -312,17 +337,121 @@ void OptimizationGuideHintsManager::ListenForNextUpdateForTesting(
   next_update_closure_ = std::move(next_update_closure);
 }
 
+void OptimizationGuideHintsManager::SetClockForTesting(
+    const base::Clock* clock) {
+  clock_ = clock;
+}
+
+void OptimizationGuideHintsManager::SetHintsFetcherForTesting(
+    std::unique_ptr<optimization_guide::HintsFetcher> hints_fetcher) {
+  hints_fetcher_ = std::move(hints_fetcher);
+}
+
 void OptimizationGuideHintsManager::MaybeScheduleHintsFetch() {
-  bool hints_fetching_allowed =
-      optimization_guide::features::IsHintsFetchingEnabled() &&
-      top_host_provider_;
-  // This local histogram is only used for testing and will be removed when the
-  // actual implementation to schedule hints fetches is in place.
-  LOCAL_HISTOGRAM_BOOLEAN("OptimizationGuide.HintsFetching.Allowed",
-                          hints_fetching_allowed);
-
-  // TODO(crbug/969558): Implement this to actually schedule a hints fetch and
-  // remove above local histogram.
+  if (!optimization_guide::features::IsHintsFetchingEnabled() ||
+      !top_host_provider_) {
+    return;
+  }
+
+  if (optimization_guide::switches::ShouldOverrideFetchHintsTimer()) {
+    SetLastHintsFetchAttemptTime(clock_->Now());
+    FetchHints();
+  } else {
+    ScheduleHintsFetch();
+  }
+}
+
+void OptimizationGuideHintsManager::ScheduleHintsFetch() {
+  DCHECK(!hints_fetch_timer_.IsRunning());
+
+  const base::TimeDelta time_until_update_time =
+      hint_cache_->FetchedHintsUpdateTime() - clock_->Now();
+  const base::TimeDelta time_until_retry =
+      GetLastHintsFetchAttemptTime() + kFetchRetryDelay - clock_->Now();
+  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|.
+    SetLastHintsFetchAttemptTime(clock_->Now());
+    hints_fetch_timer_.Start(FROM_HERE, RandomFetchDelay(), this,
+                             &OptimizationGuideHintsManager::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.
+      fetcher_delay = time_until_update_time;
+    } else {
+      // Otherwise, hints need to be updated but an attempt was made in last
+      // |kFetchRetryDelay|. Schedule the timer for after the retry
+      // delay.
+      fetcher_delay = time_until_retry;
+    }
+    hints_fetch_timer_.Start(
+        FROM_HERE, fetcher_delay, this,
+        &OptimizationGuideHintsManager::ScheduleHintsFetch);
+  }
+}
+
+void OptimizationGuideHintsManager::FetchHints() {
+  DCHECK(top_host_provider_);
+
+  size_t max_hints_to_fetch = optimization_guide::features::
+      MaxHostsForOptimizationGuideServiceHintsFetch();
+  std::vector<std::string> top_hosts =
+      top_host_provider_->GetTopHosts(max_hints_to_fetch);
+  if (top_hosts.empty())
+    return;
+  DCHECK_GE(max_hints_to_fetch, top_hosts.size());
+
+  if (!hints_fetcher_) {
+    hints_fetcher_ = std::make_unique<optimization_guide::HintsFetcher>(
+        url_loader_factory_,
+        optimization_guide::features::GetOptimizationGuideServiceURL());
+  }
+  hints_fetcher_->FetchOptimizationGuideServiceHints(
+      top_hosts, base::BindOnce(&OptimizationGuideHintsManager::OnHintsFetched,
+                                ui_weak_ptr_factory_.GetWeakPtr()));
+}
+
+void OptimizationGuideHintsManager::OnHintsFetched(
+    base::Optional<std::unique_ptr<optimization_guide::proto::GetHintsResponse>>
+        get_hints_response) {
+  if (get_hints_response) {
+    hint_cache_->UpdateFetchedHints(
+        std::move(*get_hints_response),
+        clock_->Now() + kUpdateFetchedHintsDelay,
+        base::BindOnce(&OptimizationGuideHintsManager::OnFetchedHintsStored,
+                       ui_weak_ptr_factory_.GetWeakPtr()));
+  } else {
+    // The fetch did not succeed so we will schedule to retry the fetch in
+    // after delaying for |kFetchRetryDelay|
+    // TODO(mcrouse): When the store is refactored from closures, the timer will
+    // be scheduled on failure of the store instead.
+    hints_fetch_timer_.Start(
+        FROM_HERE, kFetchRetryDelay, this,
+        &OptimizationGuideHintsManager::ScheduleHintsFetch);
+  }
+}
+
+void OptimizationGuideHintsManager::OnFetchedHintsStored() {
+  hints_fetch_timer_.Stop();
+  hints_fetch_timer_.Start(
+      FROM_HERE, hint_cache_->FetchedHintsUpdateTime() - clock_->Now(), this,
+      &OptimizationGuideHintsManager::ScheduleHintsFetch);
+}
+
+base::Time OptimizationGuideHintsManager::GetLastHintsFetchAttemptTime() const {
+  return base::Time::FromDeltaSinceWindowsEpoch(
+      base::TimeDelta::FromMicroseconds(pref_service_->GetInt64(
+          optimization_guide::prefs::kHintsFetcherLastFetchAttempt)));
+}
+
+void OptimizationGuideHintsManager::SetLastHintsFetchAttemptTime(
+    base::Time last_attempt_time) {
+  pref_service_->SetInt64(
+      optimization_guide::prefs::kHintsFetcherLastFetchAttempt,
+      last_attempt_time.ToDeltaSinceWindowsEpoch().InMicroseconds());
 }
 
 void OptimizationGuideHintsManager::LoadHintForNavigation(

chrome/browser/optimization_guide/optimization_guide_hints_manager.h

@@ -11,10 +11,13 @@
 #include <vector>
 
 #include "base/callback_forward.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/sequenced_task_runner.h"
 #include "base/synchronization/lock.h"
+#include "base/time/clock.h"
+#include "base/timer/timer.h"
 #include "components/optimization_guide/hints_component_info.h"
 #include "components/optimization_guide/optimization_guide_service_observer.h"
 #include "components/optimization_guide/proto/hints.pb.h"
@@ -31,9 +34,14 @@ namespace leveldb_proto {
 class ProtoDatabaseProvider;
 }  // namespace leveldb_proto
 
+namespace network {
+class SharedURLLoaderFactory;
+}  // namespace network
+
 namespace optimization_guide {
 class HintCache;
 class HintUpdateData;
+class HintsFetcher;
 class OptimizationFilter;
 class OptimizationGuideService;
 class TopHostProvider;
@@ -49,7 +57,8 @@ class OptimizationGuideHintsManager
       const base::FilePath& profile_path,
       PrefService* pref_service,
       leveldb_proto::ProtoDatabaseProvider* database_provider,
-      optimization_guide::TopHostProvider* top_host_provider);
+      optimization_guide::TopHostProvider* top_host_provider,
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
 
   ~OptimizationGuideHintsManager() override;
 
@@ -87,6 +96,18 @@ class OptimizationGuideHintsManager
   bool HasLoadedOptimizationFilter(
       optimization_guide::proto::OptimizationType optimization_type);
 
+  // Overrides |hints_fetcher_| for testing.
+  void SetHintsFetcherForTesting(
+      std::unique_ptr<optimization_guide::HintsFetcher> hints_fetcher);
+
+  // Returns the current hints fetcher.
+  optimization_guide::HintsFetcher* hints_fetcher() const {
+    return hints_fetcher_.get();
+  }
+
+  // Overrides |clock_| for testing.
+  void SetClockForTesting(const base::Clock* clock);
+
  private:
   // Processes the hints component.
   //
@@ -123,9 +144,36 @@ class OptimizationGuideHintsManager
   void OnComponentHintsUpdated(base::OnceClosure update_closure,
                                bool hints_updated) const;
 
-  // Method to request new hints for user's sites.
+  // Method to decide whether to fetch new hints for user's top sites and
+  // proceeds to schedule the fetch.
   void MaybeScheduleHintsFetch();
 
+  // Schedules |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.
+  void ScheduleHintsFetch();
+
+  // Called to make a request to fetch hints from the remote Optimization Guide
+  // Service.
+  void FetchHints();
+
+  // Called when the hints have been fetched from the remote Optimization Guide
+  // Service and are ready for parsing.
+  void OnHintsFetched(
+      base::Optional<
+          std::unique_ptr<optimization_guide::proto::GetHintsResponse>>
+          get_hints_response);
+
+  // Called when the fetched hints have been stored in |hint_cache| and are
+  // ready to be used.
+  void OnFetchedHintsStored();
+
+  // Returns the time when a hints fetch request was last attempted.
+  base::Time GetLastHintsFetchAttemptTime() const;
+
+  // Sets the time when a hints fetch was last attempted to |last_attempt_time|.
+  void SetLastHintsFetchAttemptTime(base::Time last_attempt_time);
+
   // Called when the request to load a hint has completed.
   void OnHintLoaded(base::OnceClosure callback,
                     const optimization_guide::proto::Hint* loaded_hint) const;
@@ -169,9 +217,25 @@ class OptimizationGuideHintsManager
   // fetched from the remote Optimization Guide Service.
   std::unique_ptr<optimization_guide::HintCache> hint_cache_;
 
+  // The fetcher that handles making requests to update hints from the remote
+  // Optimization Guide Service.
+  std::unique_ptr<optimization_guide::HintsFetcher> hints_fetcher_;
+
   // The top host provider that can be queried. Not owned.
   optimization_guide::TopHostProvider* top_host_provider_ = nullptr;
 
+  // The URL loader factory used for fetching hints from the remote Optimization
+  // Guide Service.
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
+
+  // The timer used to schedule fetching hints from the remote Optimization
+  // Guide Service.
+  base::OneShotTimer hints_fetch_timer_;
+
+  // The clock used to schedule fetching from the remote Optimization Guide
+  // Service.
+  const base::Clock* clock_;
+
   // Used in testing to subscribe to an update event in this class.
   base::OnceClosure next_update_closure_;
 

chrome/browser/optimization_guide/optimization_guide_hints_manager_unittest.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/optimization_guide/optimization_guide_hints_manager.h"
 
 #include <string>
+#include <utility>
 
 #include "base/base64.h"
 #include "base/command_line.h"
@@ -12,19 +13,30 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "components/optimization_guide/bloom_filter.h"
-#include "components/optimization_guide/command_line_top_host_provider.h"
 #include "components/optimization_guide/hints_component_util.h"
+#include "components/optimization_guide/hints_fetcher.h"
 #include "components/optimization_guide/optimization_guide_features.h"
 #include "components/optimization_guide/optimization_guide_prefs.h"
 #include "components/optimization_guide/optimization_guide_service.h"
 #include "components/optimization_guide/optimization_guide_switches.h"
 #include "components/optimization_guide/proto_database_provider_test_base.h"
+#include "components/optimization_guide/top_host_provider.h"
 #include "components/prefs/testing_pref_service.h"
 #include "content/public/test/mock_navigation_handle.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
+#include "services/network/test/test_url_loader_factory.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
+// Retry delay is 16 minutes to allow for kFetchRetryDelaySecs +
+// kFetchRandomMaxDelaySecs to pass.
+constexpr int kTestFetchRetryDelaySecs = 60 * 16;
+constexpr int kUpdateFetchHintsTimeSecs = 24 * 60 * 60;  // 24 hours.
+
 const int kBlackBlacklistBloomFilterNumHashFunctions = 7;
 const int kBlackBlacklistBloomFilterNumBits = 511;
 
@@ -53,6 +65,22 @@ void AddBlacklistBloomFilterToConfig(
   blacklist_proto->set_allocated_bloom_filter(bloom_filter_proto.release());
 }
 
+std::unique_ptr<optimization_guide::proto::GetHintsResponse> BuildHintsResponse(
+    std::vector<std::string> hosts) {
+  std::unique_ptr<optimization_guide::proto::GetHintsResponse>
+      get_hints_response =
+          std::make_unique<optimization_guide::proto::GetHintsResponse>();
+
+  for (const auto& host : hosts) {
+    optimization_guide::proto::Hint* hint = get_hints_response->add_hints();
+    hint->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
+    hint->set_key(host);
+    optimization_guide::proto::PageHint* page_hint = hint->add_page_hints();
+    page_hint->set_page_pattern("page pattern");
+  }
+  return get_hints_response;
+}
+
 }  // namespace
 
 class TestOptimizationGuideService
@@ -82,6 +110,57 @@ class TestOptimizationGuideService
   bool remove_observer_called_ = false;
 };
 
+// A mock class implementation of TopHostProvider.
+class MockTopHostProvider : public optimization_guide::TopHostProvider {
+ public:
+  MOCK_METHOD1(GetTopHosts, std::vector<std::string>(size_t max_sites));
+};
+
+enum class HintsFetcherEndState {
+  kFetchFailed = 0,
+  kFetchSuccessWithHints = 1,
+  kFetchSuccessWithNoHints = 2,
+};
+
+// A mock class implementation of HintsFetcher.
+class TestHintsFetcher : public optimization_guide::HintsFetcher {
+  using HintsFetcher::FetchOptimizationGuideServiceHints;
+
+ public:
+  TestHintsFetcher(
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+      GURL optimization_guide_service_url,
+      HintsFetcherEndState fetch_state)
+      : HintsFetcher(url_loader_factory, optimization_guide_service_url),
+        fetch_state_(fetch_state) {}
+
+  bool FetchOptimizationGuideServiceHints(
+      const std::vector<std::string>& hosts,
+      optimization_guide::HintsFetchedCallback hints_fetched_callback)
+      override {
+    switch (fetch_state_) {
+      case HintsFetcherEndState::kFetchFailed:
+        std::move(hints_fetched_callback).Run(base::nullopt);
+        return false;
+      case HintsFetcherEndState::kFetchSuccessWithHints:
+        hints_fetched_ = true;
+        std::move(hints_fetched_callback).Run(BuildHintsResponse({"host.com"}));
+        return true;
+      case HintsFetcherEndState::kFetchSuccessWithNoHints:
+        hints_fetched_ = true;
+        std::move(hints_fetched_callback).Run(BuildHintsResponse({}));
+        return true;
+    }
+    return true;
+  }
+
+  bool hints_fetched() { return hints_fetched_; }
+
+ private:
+  bool hints_fetched_ = false;
+  HintsFetcherEndState fetch_state_;
+};
+
 class OptimizationGuideHintsManagerTest
     : public optimization_guide::ProtoDatabaseProviderTestBase {
  public:
@@ -109,9 +188,14 @@ class OptimizationGuideHintsManagerTest
     pref_service_ = std::make_unique<TestingPrefServiceSimple>();
     optimization_guide::prefs::RegisterProfilePrefs(pref_service_->registry());
 
+    url_loader_factory_ =
+        base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+            &test_url_loader_factory_);
+
     hints_manager_ = std::make_unique<OptimizationGuideHintsManager>(
         optimization_guide_service_.get(), temp_dir(), pref_service_.get(),
-        db_provider_.get(), top_host_provider);
+        db_provider_.get(), top_host_provider, url_loader_factory_);
+    hints_manager_->SetClockForTesting(browser_thread_bundle_.GetMockClock());
 
     // Add observer is called after the HintCache is fully initialized,
     // indicating that the OptimizationGuideHintsManager is ready to process
@@ -171,10 +255,27 @@ class OptimizationGuideHintsManagerTest
     ProcessHints(config, version);
   }
 
+  std::unique_ptr<TestHintsFetcher> BuildTestHintsFetcher(
+      HintsFetcherEndState end_state) {
+    std::unique_ptr<TestHintsFetcher> hints_fetcher =
+        std::make_unique<TestHintsFetcher>(
+            url_loader_factory_, GURL("https://hintsserver.com"), end_state);
+    return hints_fetcher;
+  }
+
+  void MoveClockForwardBy(base::TimeDelta time_delta) {
+    browser_thread_bundle_.FastForwardBy(time_delta);
+    RunUntilIdle();
+  }
+
   OptimizationGuideHintsManager* hints_manager() const {
     return hints_manager_.get();
   }
 
+  TestHintsFetcher* hints_fetcher() const {
+    return static_cast<TestHintsFetcher*>(hints_manager()->hints_fetcher());
+  }
+
   GURL url_with_hints() const {
     return GURL("https://somedomain.org/news/whatever");
   }
@@ -199,11 +300,15 @@ class OptimizationGuideHintsManagerTest
                               serialized_config.size()));
   }
 
-  content::TestBrowserThreadBundle browser_thread_bundle_;
+  content::TestBrowserThreadBundle browser_thread_bundle_ = {
+      base::test::ScopedTaskEnvironment::MainThreadType::UI,
+      base::test::ScopedTaskEnvironment::TimeSource::MOCK_TIME};
 
   std::unique_ptr<OptimizationGuideHintsManager> hints_manager_;
   std::unique_ptr<TestOptimizationGuideService> optimization_guide_service_;
   std::unique_ptr<TestingPrefServiceSimple> pref_service_;
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
+  network::TestURLLoaderFactory test_url_loader_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(OptimizationGuideHintsManagerTest);
 };
@@ -711,46 +816,157 @@ TEST_F(OptimizationGuideHintsManagerTest,
   feature_list.InitWithFeatures(
       {optimization_guide::features::kOptimizationHintsFetching}, {});
 
-  base::HistogramTester histogram_tester;
+  std::unique_ptr<MockTopHostProvider> top_host_provider =
+      std::make_unique<MockTopHostProvider>();
+  EXPECT_CALL(*top_host_provider, GetTopHosts(testing::_)).Times(0);
 
   CreateServiceAndHintsManager(/*top_host_provider=*/nullptr);
+  hints_manager()->SetHintsFetcherForTesting(
+      BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithHints));
 
-  histogram_tester.ExpectUniqueSample("OptimizationGuide.HintsFetching.Allowed",
-                                      false, 1);
+  // Force timer to expire and schedule a hints fetch.
+  MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
+  EXPECT_FALSE(hints_fetcher()->hints_fetched());
 }
 
 TEST_F(OptimizationGuideHintsManagerTest,
        HintsFetchNotAllowedIfFeatureIsNotEnabledButTopHostProviderIsProvided) {
-  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
-      optimization_guide::switches::kFetchHintsOverride, "whatever.com");
   base::test::ScopedFeatureList feature_list;
   feature_list.InitWithFeatures(
       {}, {optimization_guide::features::kOptimizationHintsFetching});
 
-  base::HistogramTester histogram_tester;
+  std::unique_ptr<MockTopHostProvider> top_host_provider =
+      std::make_unique<MockTopHostProvider>();
+  EXPECT_CALL(*top_host_provider, GetTopHosts(testing::_)).Times(0);
 
-  std::unique_ptr<optimization_guide::TopHostProvider> top_host_provider =
-      optimization_guide::CommandLineTopHostProvider::CreateIfEnabled();
   CreateServiceAndHintsManager(top_host_provider.get());
-
-  histogram_tester.ExpectUniqueSample("OptimizationGuide.HintsFetching.Allowed",
-                                      false, 1);
 }
 
 TEST_F(OptimizationGuideHintsManagerTest,
        HintsFetchAllowedIfFeatureIsEnabledAndTopHostProviderIsProvided) {
-  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
-      optimization_guide::switches::kFetchHintsOverride, "whatever.com");
   base::test::ScopedFeatureList feature_list;
   feature_list.InitWithFeatures(
       {optimization_guide::features::kOptimizationHintsFetching}, {});
 
-  base::HistogramTester histogram_tester;
+  std::unique_ptr<MockTopHostProvider> top_host_provider =
+      std::make_unique<MockTopHostProvider>();
+  std::vector<std::string> hosts = {"example1.com", "example2.com"};
+  EXPECT_CALL(*top_host_provider, GetTopHosts(testing::_))
+      .Times(1)
+      .WillRepeatedly(testing::Return(hosts));
 
-  std::unique_ptr<optimization_guide::TopHostProvider> top_host_provider =
-      optimization_guide::CommandLineTopHostProvider::CreateIfEnabled();
   CreateServiceAndHintsManager(top_host_provider.get());
+  hints_manager()->SetHintsFetcherForTesting(
+      BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithHints));
 
-  histogram_tester.ExpectUniqueSample("OptimizationGuide.HintsFetching.Allowed",
-                                      true, 1);
+  // Force timer to expire and schedule a hints fetch.
+  MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
+  EXPECT_TRUE(hints_fetcher()->hints_fetched());
+}
+
+TEST_F(OptimizationGuideHintsManagerTest, HintsFetcherEnabledNoHostsToFetch) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      optimization_guide::features::kOptimizationHintsFetching);
+
+  std::unique_ptr<MockTopHostProvider> top_host_provider =
+      std::make_unique<MockTopHostProvider>();
+  EXPECT_CALL(*top_host_provider, GetTopHosts(testing::_)).Times(1);
+  CreateServiceAndHintsManager(top_host_provider.get());
+  hints_manager()->SetHintsFetcherForTesting(
+      BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithHints));
+
+  // Force timer to expire and schedule a hints fetch.
+  MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
+  EXPECT_FALSE(hints_fetcher()->hints_fetched());
+}
+
+TEST_F(OptimizationGuideHintsManagerTest,
+       HintsFetcherEnabledWithHostsNoHintsInResponse) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      optimization_guide::features::kOptimizationHintsFetching);
+
+  std::unique_ptr<MockTopHostProvider> top_host_provider =
+      std::make_unique<MockTopHostProvider>();
+  std::vector<std::string> hosts = {"example1.com", "example2.com"};
+  // This should be called exactly once, confirming that hints are not fetched
+  // again after |kTestFetchRetryDelaySecs|.
+  EXPECT_CALL(*top_host_provider, GetTopHosts(testing::_))
+      .Times(1)
+      .WillRepeatedly(testing::Return(hosts));
+  CreateServiceAndHintsManager(top_host_provider.get());
+  hints_manager()->SetHintsFetcherForTesting(
+      BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithNoHints));
+
+  // Force timer to expire and schedule a hints fetch.
+  MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
+  EXPECT_TRUE(hints_fetcher()->hints_fetched());
+
+  // Check that hints should not be fetched again after the delay for a failed
+  // hints fetch attempt.
+  MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
+  EXPECT_CALL(*top_host_provider, GetTopHosts(testing::_)).Times(0);
+}
+
+TEST_F(OptimizationGuideHintsManagerTest, HintsFetcherTimerRetryDelay) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      optimization_guide::features::kOptimizationHintsFetching);
+
+  std::unique_ptr<MockTopHostProvider> top_host_provider =
+      std::make_unique<MockTopHostProvider>();
+  std::vector<std::string> hosts = {"example1.com", "example2.com"};
+  // Should be called twice: once for the failed fetch and then again for the
+  // successful fetch.
+  EXPECT_CALL(*top_host_provider, GetTopHosts(testing::_))
+      .Times(2)
+      .WillRepeatedly(testing::Return(hosts));
+
+  CreateServiceAndHintsManager(top_host_provider.get());
+  hints_manager()->SetHintsFetcherForTesting(
+      BuildTestHintsFetcher(HintsFetcherEndState::kFetchFailed));
+
+  // Force timer to expire and schedule a hints fetch - first time.
+  MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
+  EXPECT_FALSE(hints_fetcher()->hints_fetched());
+
+  // Force speculative timer to expire after fetch fails first time, update
+  // hints fetcher so it succeeds this time.
+  hints_manager()->SetHintsFetcherForTesting(
+      BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithHints));
+  MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
+  EXPECT_TRUE(hints_fetcher()->hints_fetched());
+}
+
+TEST_F(OptimizationGuideHintsManagerTest, HintsFetcherTimerFetchSucceeds) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      optimization_guide::features::kOptimizationHintsFetching);
+
+  std::unique_ptr<MockTopHostProvider> top_host_provider =
+      std::make_unique<MockTopHostProvider>();
+  std::vector<std::string> hosts = {"example1.com", "example2.com"};
+  EXPECT_CALL(*top_host_provider, GetTopHosts(testing::_))
+      .WillRepeatedly(testing::Return(hosts));
+
+  // Force hints fetch scheduling.
+  CreateServiceAndHintsManager(top_host_provider.get());
+  hints_manager()->SetHintsFetcherForTesting(
+      BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithHints));
+
+  // Force timer to expire and schedule a hints fetch that succeeds.
+  MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
+  EXPECT_TRUE(hints_fetcher()->hints_fetched());
+
+  // TODO(mcrouse): Make sure timer is triggered by metadata entry,
+  // |hint_cache| control needed.
+  hints_manager()->SetHintsFetcherForTesting(
+      BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithHints));
+
+  MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
+  EXPECT_FALSE(hints_fetcher()->hints_fetched());
+
+  MoveClockForwardBy(base::TimeDelta::FromSeconds(kUpdateFetchHintsTimeSecs));
+  EXPECT_TRUE(hints_fetcher()->hints_fetched());
 }

chrome/browser/optimization_guide/optimization_guide_keyed_service.cc

@@ -17,6 +17,7 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/storage_partition.h"
 
 namespace {
 
@@ -57,10 +58,12 @@ void OptimizationGuideKeyedService::Initialize(
   DCHECK(optimization_guide_service);
 
   top_host_provider_ = GetTopHostProviderIfUserPermitted(browser_context_);
+  Profile* profile = Profile::FromBrowserContext(browser_context_);
   hints_manager_ = std::make_unique<OptimizationGuideHintsManager>(
-      optimization_guide_service, profile_path,
-      Profile::FromBrowserContext(browser_context_)->GetPrefs(),
-      database_provider, top_host_provider_.get());
+      optimization_guide_service, profile_path, profile->GetPrefs(),
+      database_provider, top_host_provider_.get(),
+      content::BrowserContext::GetDefaultStoragePartition(profile)
+          ->GetURLLoaderFactoryForBrowserProcess());
 }
 
 void OptimizationGuideKeyedService::RegisterOptimizationTypes(

components/optimization_guide/hints_fetcher.h

@@ -24,6 +24,12 @@ class SimpleURLLoader;
 
 namespace optimization_guide {
 
+// Callback to inform the caller that the remote hints have been fetched and
+// to pass back the fetched hints response from the remote Optimization Guide
+// Service.
+using HintsFetchedCallback = base::OnceCallback<void(
+    base::Optional<std::unique_ptr<proto::GetHintsResponse>>)>;
+
 // A class to handle requests for optimization hints from a remote Optimization
 // Guide Service.
 //
@@ -31,12 +37,6 @@ namespace optimization_guide {
 // Owner must ensure that |hint_cache| remains alive for the lifetime of
 // |HintsFetcher|.
 class HintsFetcher {
-  // Callback to inform the caller that the remote hints have been fetched and
-  // to pass back the fetched hints response from the remote Optimization Guide
-  // Service.
-  using HintsFetchedCallback = base::OnceCallback<void(
-      base::Optional<std::unique_ptr<proto::GetHintsResponse>>)>;
-
  public:
   HintsFetcher(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,

components/previews/content/previews_optimization_guide_unittest.cc

@@ -5,6 +5,7 @@
 #include "components/previews/content/previews_optimization_guide.h"
 
 #include <memory>
+#include <utility>
 
 #include "base/base64.h"
 #include "base/bind.h"
@@ -120,11 +121,6 @@ std::unique_ptr<optimization_guide::proto::GetHintsResponse> BuildHintsResponse(
 // A mock class implementation of HintsFetcher for unittesting
 // previews_optimization_guide.
 class TestHintsFetcher : public optimization_guide::HintsFetcher {
-  using HintsFetchedCallback = base::OnceCallback<void(
-      base::Optional<
-          std::unique_ptr<optimization_guide::proto::GetHintsResponse>>)>;
-  using HintsFetcher::FetchOptimizationGuideServiceHints;
-
  public:
   TestHintsFetcher(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
@@ -135,7 +131,8 @@ class TestHintsFetcher : public optimization_guide::HintsFetcher {
 
   bool FetchOptimizationGuideServiceHints(
       const std::vector<std::string>& hosts,
-      HintsFetchedCallback hints_fetched_callback) override {
+      optimization_guide::HintsFetchedCallback hints_fetched_callback)
+      override {
     switch (fetch_state_) {
       case HintsFetcherEndState::kFetchFailed:
         std::move(hints_fetched_callback).Run(base::nullopt);