Remove retries for batch update hints fetching

Given that we race on every navigation, it doesn't make sense to retry
the batch update hints fetching since we'll just fetch for any necessary
failures on navigation anyway.

Change-Id: I6d96d5d6ac34252e80f36ba8c7732fb1e1e3ff9c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2426972Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809992}

chrome/browser/optimization_guide/optimization_guide_hints_manager.cc

@@ -64,10 +64,6 @@ 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 =
@@ -620,28 +616,18 @@ void OptimizationGuideHintsManager::ScheduleTopHostsHintsFetch() {
 
   const base::TimeDelta time_until_update_time =
       hint_cache_->GetFetchedHintsUpdateTime() - 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()) {
+  if (time_until_update_time <= base::TimeDelta()) {
     // Fetched hints in the store should be updated and an attempt has not
-    // been made in last |kFetchRetryDelay|.
+    // been made in last |kUpdateFetchedHintsDelay|.
     SetLastHintsFetchAttemptTime(clock_->Now());
     top_hosts_hints_fetch_timer_.Start(
         FROM_HERE, RandomFetchDelay(), this,
         &OptimizationGuideHintsManager::FetchTopHostsHints);
   } 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;
-    }
     top_hosts_hints_fetch_timer_.Start(
         FROM_HERE, fetcher_delay, this,
         &OptimizationGuideHintsManager::ScheduleTopHostsHintsFetch);
@@ -676,23 +662,16 @@ void OptimizationGuideHintsManager::OnTopHostsHintsFetched(
     const base::flat_set<std::string>& hosts_fetched,
     base::Optional<std::unique_ptr<optimization_guide::proto::GetHintsResponse>>
         get_hints_response) {
-  if (get_hints_response) {
+  if (!get_hints_response)
+    return;
+
   hint_cache_->UpdateFetchedHints(
-        std::move(*get_hints_response),
-        clock_->Now() + kUpdateFetchedHintsDelay, hosts_fetched,
+      std::move(*get_hints_response), clock_->Now() + kUpdateFetchedHintsDelay,
+      hosts_fetched,
       /*urls_fetched=*/{},
       base::BindOnce(
           &OptimizationGuideHintsManager::OnFetchedTopHostsHintsStored,
           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.
-    top_hosts_hints_fetch_timer_.Start(
-        FROM_HERE, kFetchRetryDelay, this,
-        &OptimizationGuideHintsManager::ScheduleTopHostsHintsFetch);
-  }
 }
 
 void OptimizationGuideHintsManager::OnPageNavigationHintsFetched(

chrome/browser/optimization_guide/optimization_guide_hints_manager_unittest.cc

@@ -2177,8 +2177,8 @@ TEST_F(OptimizationGuideHintsManagerFetchingTest,
   EXPECT_EQ(1, top_host_provider->get_num_top_hosts_called());
   EXPECT_EQ(1, batch_update_hints_fetcher()->num_fetches_requested());
 
-  // Check that hints should not be fetched again after the delay for a failed
-  // hints fetch attempt.
+  // Check that hints should not be fetched again after the delay for a hints
+  // fetch attempt with no hints.
   MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
   // This should be called exactly once, confirming that hints are not fetched
   // again after |kTestFetchRetryDelaySecs|.
@@ -2196,9 +2196,7 @@ TEST_F(OptimizationGuideHintsManagerFetchingTest, HintsFetcherTimerRetryDelay) {
   CreateServiceAndHintsManager({optimization_guide::proto::DEFER_ALL_SCRIPT},
                                top_host_provider.get());
   hints_manager()->SetHintsFetcherFactoryForTesting(
-      BuildTestHintsFetcherFactory(
-          {HintsFetcherEndState::kFetchFailed,
-           HintsFetcherEndState::kFetchSuccessWithHostHints}));
+      BuildTestHintsFetcherFactory({HintsFetcherEndState::kFetchFailed}));
   InitializeWithDefaultConfig("1.0.0");
 
   // Force timer to expire and schedule a hints fetch - first time.
@@ -2206,11 +2204,10 @@ TEST_F(OptimizationGuideHintsManagerFetchingTest, HintsFetcherTimerRetryDelay) {
   EXPECT_EQ(1, top_host_provider->get_num_top_hosts_called());
   EXPECT_EQ(1, batch_update_hints_fetcher()->num_fetches_requested());
 
-  // Force speculative timer to expire after fetch fails first time, update
-  // hints fetcher so it succeeds this time.
+  // Force speculative timer to expire after fetch fails.
   MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
-  EXPECT_EQ(2, top_host_provider->get_num_top_hosts_called());
-  EXPECT_EQ(2, batch_update_hints_fetcher()->num_fetches_requested());
+  EXPECT_EQ(1, top_host_provider->get_num_top_hosts_called());
+  EXPECT_EQ(1, batch_update_hints_fetcher()->num_fetches_requested());
 }
 
 TEST_F(OptimizationGuideHintsManagerFetchingTest,