Move hints fetching to after component has arrived in OGKS

This is pretty much guaranteed for DataSaver users to always happen
and this reduces the likelihood of the race condition between data
updates being in flight

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

chrome/browser/optimization_guide/optimization_guide_hints_manager.cc

@@ -300,8 +300,6 @@ void OptimizationGuideHintsManager::OnHintCacheInitialized() {
     UpdateComponentHints(base::DoNothing(), std::move(hint_update_data));
   }
 
-  MaybeScheduleHintsFetch();
-
   // Register as an observer regardless of hint proto override usage. This is
   // needed as a signal during testing.
   optimization_guide_service_->AddObserver(this);
@@ -330,7 +328,7 @@ void OptimizationGuideHintsManager::UpdateComponentHints(
 
 void OptimizationGuideHintsManager::OnComponentHintsUpdated(
     base::OnceClosure update_closure,
-    bool hints_updated) const {
+    bool hints_updated) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // Record the result of updating the hints. This is used as a signal for the
@@ -339,6 +337,8 @@ void OptimizationGuideHintsManager::OnComponentHintsUpdated(
       optimization_guide::kComponentHintsUpdatedResultHistogramString,
       hints_updated);
 
+  MaybeScheduleHintsFetch();
+
   MaybeRunUpdateClosure(std::move(update_closure));
 }
 

chrome/browser/optimization_guide/optimization_guide_hints_manager.h

@@ -162,7 +162,7 @@ class OptimizationGuideHintsManager
   // Called when the hints have been fully updated with the latest hints from
   // the Component Updater. This is used as a signal during tests.
   void OnComponentHintsUpdated(base::OnceClosure update_closure,
-                               bool hints_updated) const;
+                               bool hints_updated);
 
   // Method to decide whether to fetch new hints for user's top sites and
   // proceeds to schedule the fetch.

chrome/browser/optimization_guide/optimization_guide_hints_manager_unittest.cc

@@ -844,6 +844,7 @@ TEST_F(OptimizationGuideHintsManagerTest,
   CreateServiceAndHintsManager(/*top_host_provider=*/nullptr);
   hints_manager()->SetHintsFetcherForTesting(
       BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithHints));
+  InitializeWithDefaultConfig("1.0.0");
 
   // Force timer to expire and schedule a hints fetch.
   MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
@@ -861,6 +862,7 @@ TEST_F(OptimizationGuideHintsManagerTest,
   EXPECT_CALL(*top_host_provider, GetTopHosts(testing::_)).Times(0);
 
   CreateServiceAndHintsManager(top_host_provider.get());
+  InitializeWithDefaultConfig("1.0.0");
 }
 
 TEST_F(OptimizationGuideHintsManagerTest,
@@ -879,6 +881,7 @@ TEST_F(OptimizationGuideHintsManagerTest,
   CreateServiceAndHintsManager(top_host_provider.get());
   hints_manager()->SetHintsFetcherForTesting(
       BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithHints));
+  InitializeWithDefaultConfig("1.0.0");
 
   // Force timer to expire and schedule a hints fetch.
   MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
@@ -896,6 +899,7 @@ TEST_F(OptimizationGuideHintsManagerTest, HintsFetcherEnabledNoHostsToFetch) {
   CreateServiceAndHintsManager(top_host_provider.get());
   hints_manager()->SetHintsFetcherForTesting(
       BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithHints));
+  InitializeWithDefaultConfig("1.0.0");
 
   // Force timer to expire and schedule a hints fetch.
   MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
@@ -919,6 +923,7 @@ TEST_F(OptimizationGuideHintsManagerTest,
   CreateServiceAndHintsManager(top_host_provider.get());
   hints_manager()->SetHintsFetcherForTesting(
       BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithNoHints));
+  InitializeWithDefaultConfig("1.0.0");
 
   // Force timer to expire and schedule a hints fetch.
   MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
@@ -947,6 +952,7 @@ TEST_F(OptimizationGuideHintsManagerTest, HintsFetcherTimerRetryDelay) {
   CreateServiceAndHintsManager(top_host_provider.get());
   hints_manager()->SetHintsFetcherForTesting(
       BuildTestHintsFetcher(HintsFetcherEndState::kFetchFailed));
+  InitializeWithDefaultConfig("1.0.0");
 
   // Force timer to expire and schedule a hints fetch - first time.
   MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
@@ -975,6 +981,7 @@ TEST_F(OptimizationGuideHintsManagerTest, HintsFetcherTimerFetchSucceeds) {
   CreateServiceAndHintsManager(top_host_provider.get());
   hints_manager()->SetHintsFetcherForTesting(
       BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithHints));
+  InitializeWithDefaultConfig("1.0.0");
 
   // Force timer to expire and schedule a hints fetch that succeeds.
   MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));

chrome/browser/optimization_guide/optimization_guide_keyed_service_browsertest.cc

@@ -499,6 +499,12 @@ class OptimizationGuideKeyedServiceHintsFetcherTest
     OptimizationGuideKeyedServiceCommandLineOverridesTest::SetUpCommandLine(
         cmd);
 
+    // Remove DataSaver switch in case it was added before. This ensures that
+    // the other implementation does not also run the hints fetch at the same
+    // time since we have not yet isolated the paths using the
+    // OptimizationGuideKeyedService feature yet.
+    cmd->RemoveSwitch("enable-spdy-proxy-auth");
+
     cmd->AppendSwitch(optimization_guide::switches::kFetchHintsOverrideTimer);
     cmd->AppendSwitchASCII(
         optimization_guide::switches::kOptimizationGuideServiceURL,
@@ -509,7 +515,9 @@ class OptimizationGuideKeyedServiceHintsFetcherTest
     OptimizationGuideKeyedServiceCommandLineOverridesTest::SetUpOnMainThread();
 
     api_server_->StartAcceptingConnections();
+  }
 
+  void WaitForHintsFetchToComplete() {
     // Expect that the browser initialization will record at least one sample
     // in each of the follow histograms as OnePlatform Hints are enabled.
     EXPECT_EQ(
@@ -519,7 +527,7 @@ class OptimizationGuideKeyedServiceHintsFetcherTest
         1);
 
     // There should be 2 sites passed via command line.
-    histogram_tester_.ExpectBucketCount(
+    histogram_tester_.ExpectUniqueSample(
         "OptimizationGuide.HintsFetcher.GetHintsRequest.HostCount", 2, 1);
 
     EXPECT_EQ(RetryForHistogramUntilCountReached(
@@ -593,10 +601,16 @@ class OptimizationGuideKeyedServiceHintsFetcherTest
 };
 
 // TODO(crbug/969558): Figure out why hints fetcher not fetching on ChromeOS.
-// TODO(crbug/997106): Flaky on Win, Mac and Linux.
+#if defined(OS_CHROMEOS)
+#define DISABLE_ON_CHROMEOS(x) DISABLED_##x
+#else
+#define DISABLE_ON_CHROMEOS(x) x
+#endif
+
 IN_PROC_BROWSER_TEST_F(OptimizationGuideKeyedServiceHintsFetcherTest,
-                       DISABLED_ClearFetchedHints) {
+                       DISABLE_ON_CHROMEOS(ClearFetchedHints)) {
   PushHintsComponentAndWaitForCompletion();
+  WaitForHintsFetchToComplete();
 
   RegisterWithKeyedService();