Stop tying the recording of hint after commit bit to a call to

CanApplyOptimization

This is necessary if all consumers use the async API which doesn't set
the bit.

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

chrome/browser/optimization_guide/hints_fetcher_browsertest.cc

@@ -129,32 +129,6 @@ GURL GetURLWithGoogleHost(net::EmbeddedTestServer* server,
 
 }  // namespace
 
-// A WebContentsObserver that asks whether an optimization type can be applied.
-class OptimizationGuideConsumerWebContentsObserver
-    : public content::WebContentsObserver {
- public:
-  OptimizationGuideConsumerWebContentsObserver(
-      content::WebContents* web_contents)
-      : content::WebContentsObserver(web_contents) {
-    OptimizationGuideKeyedServiceFactory::GetForProfile(
-        Profile::FromBrowserContext(web_contents->GetBrowserContext()))
-        ->RegisterOptimizationTypesAndTargets(
-            {optimization_guide::proto::NOSCRIPT}, {});
-  }
-  ~OptimizationGuideConsumerWebContentsObserver() override = default;
-
-  // contents::WebContentsObserver implementation:
-  void DidFinishNavigation(
-      content::NavigationHandle* navigation_handle) override {
-    OptimizationGuideKeyedService* service =
-        OptimizationGuideKeyedServiceFactory::GetForProfile(
-            Profile::FromBrowserContext(web_contents()->GetBrowserContext()));
-    service->CanApplyOptimization(navigation_handle,
-                                  optimization_guide::proto::NOSCRIPT,
-                                  /*optimization_metadata=*/nullptr);
-  }
-};
-
 // This test class sets up everything but does not enable any
 // HintsFetcher-related features. The parameter selects whether the
 // OptimizationGuideKeyedService is enabled (tests should pass in the same way
@@ -537,16 +511,20 @@ class HintsFetcherBrowserTest : public HintsFetcherDisabledBrowserTest {
   }
 
   void SetUpOnMainThread() override {
-    // Set up an OptimizationGuideKeyedService consumer.
-    consumer_.reset(new OptimizationGuideConsumerWebContentsObserver(
-        browser()->tab_strip_model()->GetActiveWebContents()));
+    // Register an optimization type, so hints will be fetched at page
+    // navigation.
+    OptimizationGuideKeyedServiceFactory::GetForProfile(
+        Profile::FromBrowserContext(browser()
+                                        ->tab_strip_model()
+                                        ->GetActiveWebContents()
+                                        ->GetBrowserContext()))
+        ->RegisterOptimizationTypesAndTargets(
+            {optimization_guide::proto::NOSCRIPT}, {});
 
     HintsFetcherDisabledBrowserTest::SetUpOnMainThread();
   }
 
  private:
-  std::unique_ptr<OptimizationGuideConsumerWebContentsObserver> consumer_;
-
   DISALLOW_COPY_AND_ASSIGN(HintsFetcherBrowserTest);
 };
 

chrome/browser/optimization_guide/optimization_guide_hints_manager.cc

@@ -928,23 +928,6 @@ OptimizationGuideHintsManager::CanApplyOptimization(
     return optimization_guide::OptimizationTypeDecision::kNoHintAvailable;
   const auto& host = url.host();
 
-  // Check if we have a hint already loaded for this navigation.
-  const optimization_guide::proto::Hint* loaded_hint =
-      hint_cache_->GetHostKeyedHintIfLoaded(host);
-  bool has_hint_in_cache = hint_cache_->HasHint(host);
-  const optimization_guide::proto::PageHint* matched_page_hint =
-      loaded_hint ? GetPageHint(navigation_data, url, loaded_hint) : nullptr;
-
-  // Populate navigation data with hint information, if provided.
-  if (navigation_data) {
-    navigation_data->set_has_hint_after_commit(has_hint_in_cache);
-
-    if (loaded_hint) {
-      navigation_data->set_serialized_hint_version_string(
-          loaded_hint->version());
-    }
-  }
-
   // Check if the URL should be filtered out if we have an optimization filter
   // for the type.
   {
@@ -983,11 +966,14 @@ OptimizationGuideHintsManager::CanApplyOptimization(
     }
   }
 
+  // Check if we have a hint already loaded for this navigation.
+  const optimization_guide::proto::Hint* loaded_hint =
+      hint_cache_->GetHostKeyedHintIfLoaded(host);
   if (!loaded_hint) {
     // If we do not have a hint already loaded and we do not have one in the
     // cache, we do not know what to do with the URL so just return.
     // Otherwise, we do have information, but we just do not know it yet.
-    if (has_hint_in_cache) {
+    if (hint_cache_->HasHint(host)) {
       return optimization_guide::OptimizationTypeDecision::
           kHadHintButNotLoadedInTime;
     }
@@ -1000,6 +986,8 @@ OptimizationGuideHintsManager::CanApplyOptimization(
     return optimization_guide::OptimizationTypeDecision::kNoHintAvailable;
   }
 
+  const optimization_guide::proto::PageHint* matched_page_hint =
+      loaded_hint ? GetPageHint(navigation_data, url, loaded_hint) : nullptr;
   return IsOptimizationTypeSupportedByPageHint(
              matched_page_hint, optimization_type, optimization_metadata)
              ? optimization_guide::OptimizationTypeDecision::kAllowedByHint
@@ -1093,9 +1081,9 @@ void OptimizationGuideHintsManager::OnNavigationStartOrRedirect(
     return;
   }
 
-  MaybeFetchHintsForNavigation(navigation_handle);
-
   LoadHintForNavigation(navigation_handle, std::move(callback));
+
+  MaybeFetchHintsForNavigation(navigation_handle);
 }
 
 void OptimizationGuideHintsManager::MaybeFetchHintsForNavigation(
@@ -1181,9 +1169,23 @@ void OptimizationGuideHintsManager::MaybeFetchHintsForNavigation(
 }
 
 void OptimizationGuideHintsManager::OnNavigationFinish(
-    const GURL& navigation_url) {
+    const GURL& navigation_url,
+    OptimizationGuideNavigationData* navigation_data) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  // Populate navigation data with hint information.
+  if (navigation_data && navigation_url.has_host()) {
+    const std::string host = navigation_url.host();
+    navigation_data->set_has_hint_after_commit(hint_cache_->HasHint(host));
+
+    const optimization_guide::proto::Hint* loaded_hint =
+        hint_cache_->GetHostKeyedHintIfLoaded(host);
+    if (loaded_hint) {
+      navigation_data->set_serialized_hint_version_string(
+          loaded_hint->version());
+    }
+  }
+
   // The callbacks will be invoked when the fetch request comes back, so it
   // will be cleaned up later.
   if (IsHintBeingFetchedForNavigation(navigation_url))

chrome/browser/optimization_guide/optimization_guide_hints_manager.h

@@ -156,7 +156,9 @@ class OptimizationGuideHintsManager
                                    base::OnceClosure callback);
 
   // Notifies |this| that a navigation with URL |navigation_url| has finished.
-  void OnNavigationFinish(const GURL& navigation_url);
+  // The |navigation_data| will be updated based on the current state of |this|.
+  void OnNavigationFinish(const GURL& navigation_url,
+                          OptimizationGuideNavigationData* navigation_data);
 
  private:
   FRIEND_TEST_ALL_PREFIXES(OptimizationGuideHintsManagerTest, IsGoogleURL);

chrome/browser/optimization_guide/optimization_guide_keyed_service.cc

@@ -143,11 +143,12 @@ void OptimizationGuideKeyedService::OnNavigationStartOrRedirect(
 }
 
 void OptimizationGuideKeyedService::OnNavigationFinish(
-    const GURL& navigation_url) {
+    const GURL& navigation_url,
+    OptimizationGuideNavigationData* navigation_data) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   if (hints_manager_)
-    hints_manager_->OnNavigationFinish(navigation_url);
+    hints_manager_->OnNavigationFinish(navigation_url, navigation_data);
 }
 
 void OptimizationGuideKeyedService::RegisterOptimizationTypesAndTargets(

chrome/browser/optimization_guide/optimization_guide_keyed_service.h

@@ -37,6 +37,7 @@ class PredictionManager;
 
 class GURL;
 class OptimizationGuideHintsManager;
+class OptimizationGuideNavigationData;
 
 class OptimizationGuideKeyedService
     : public KeyedService,
@@ -73,7 +74,8 @@ class OptimizationGuideKeyedService
 
   // Notifies |hints_manager_| that the navigation associated with
   // |navigation_url| has finished.
-  void OnNavigationFinish(const GURL& navigation_url);
+  void OnNavigationFinish(const GURL& navigation_url,
+                          OptimizationGuideNavigationData* navigation_data);
 
   // Clears data specific to the user.
   void ClearData();

chrome/browser/optimization_guide/optimization_guide_web_contents_observer.cc

@@ -173,7 +173,8 @@ void OptimizationGuideWebContentsObserver::
   // If we have a navigation data for it, it's probably safe to say that this
   // URL is the main frame URL of the navigation.
   if (optimization_guide_keyed_service_)
-    optimization_guide_keyed_service_->OnNavigationFinish(navigation_url);
+    optimization_guide_keyed_service_->OnNavigationFinish(
+        navigation_url, &nav_data_iter->second);
 
   (nav_data_iter->second).RecordMetrics(has_committed);