Load hints for correct hosts after a page navigation fetch request

Bug: 1036490
Change-Id: I03eeb9b6051a8b399ee2aca6522ef2c0ae5ae5f4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2013745
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734340}

chrome/browser/optimization_guide/optimization_guide_hints_manager.cc

@@ -581,18 +581,17 @@ void OptimizationGuideHintsManager::OnTopHostsHintsFetched(
 }
 
 void OptimizationGuideHintsManager::OnPageNavigationHintsFetched(
+    const base::flat_set<std::string>& page_navigation_hosts_requested,
     base::Optional<std::unique_ptr<optimization_guide::proto::GetHintsResponse>>
         get_hints_response) {
-  if (!get_hints_response.has_value() || !get_hints_response.value()) {
-    page_navigation_hosts_being_fetched_.clear();
+  if (!get_hints_response.has_value() || !get_hints_response.value())
     return;
-  }
 
   hint_cache_->UpdateFetchedHints(
       std::move(*get_hints_response), clock_->Now() + kUpdateFetchedHintsDelay,
       base::BindOnce(
           &OptimizationGuideHintsManager::OnFetchedPageNavigationHintsStored,
-          ui_weak_ptr_factory_.GetWeakPtr()));
+          ui_weak_ptr_factory_.GetWeakPtr(), page_navigation_hosts_requested));
 }
 
 void OptimizationGuideHintsManager::OnFetchedTopHostsHintsStored() {
@@ -607,12 +606,11 @@ void OptimizationGuideHintsManager::OnFetchedTopHostsHintsStored() {
       &OptimizationGuideHintsManager::ScheduleTopHostsHintsFetch);
 }
 
-void OptimizationGuideHintsManager::OnFetchedPageNavigationHintsStored() {
+void OptimizationGuideHintsManager::OnFetchedPageNavigationHintsStored(
+    const base::flat_set<std::string>& page_navigation_hosts_requested) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  for (const auto& host : page_navigation_hosts_being_fetched_)
+  for (const auto& host : page_navigation_hosts_requested)
     LoadHintForHost(host, base::DoNothing());
-
-  page_navigation_hosts_being_fetched_.clear();
 }
 
 base::Time OptimizationGuideHintsManager::GetLastHintsFetchAttemptTime() const {
@@ -688,6 +686,7 @@ void OptimizationGuideHintsManager::OnPredictionUpdated(
 
   // Extract the target hosts. Use a flat set to remove duplicates.
   // |target_hosts_serialized| is the ordered list of non-duplicate hosts.
+  // TODO(sophiechang): See if we can make this logic simpler.
   base::flat_set<std::string> target_hosts;
   std::vector<std::string> target_hosts_serialized;
   for (const auto& url : prediction->sorted_predicted_urls()) {
@@ -709,10 +708,6 @@ void OptimizationGuideHintsManager::OnPredictionUpdated(
   if (target_hosts.empty())
     return;
 
-  page_navigation_hosts_being_fetched_.clear();
-  for (const auto& host : target_hosts)
-    page_navigation_hosts_being_fetched_.push_back(host);
-
   if (!hints_fetcher_) {
     hints_fetcher_ = std::make_unique<optimization_guide::HintsFetcher>(
         url_loader_factory_,
@@ -728,7 +723,7 @@ void OptimizationGuideHintsManager::OnPredictionUpdated(
       optimization_guide::proto::CONTEXT_PAGE_NAVIGATION,
       base::BindOnce(
           &OptimizationGuideHintsManager::OnPageNavigationHintsFetched,
-          ui_weak_ptr_factory_.GetWeakPtr()));
+          ui_weak_ptr_factory_.GetWeakPtr(), target_hosts));
 
   for (const auto& host : target_hosts)
     LoadHintForHost(host, base::DoNothing());
@@ -896,7 +891,7 @@ OptimizationGuideHintsManager::CanApplyOptimization(
   const optimization_guide::proto::Hint* url_keyed_hint =
       hint_cache_->GetURLKeyedHint(url);
   if (url_keyed_hint) {
-    DCHECK(url_keyed_hint->page_hints_size() == 1);
+    DCHECK_EQ(url_keyed_hint->page_hints_size(), 1);
     if (url_keyed_hint->page_hints_size() > 0 &&
         IsOptimizationTypeSupportedByPageHint(&url_keyed_hint->page_hints(0),
                                               optimization_type,
@@ -906,10 +901,6 @@ OptimizationGuideHintsManager::CanApplyOptimization(
   }
 
   if (!loaded_hint) {
-    if (IsHintBeingFetched(url.host())) {
-      return optimization_guide::OptimizationTypeDecision::
-          kHintFetchStartedButNotAvailableInTime;
-    }
     // 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.
@@ -917,6 +908,13 @@ OptimizationGuideHintsManager::CanApplyOptimization(
       return optimization_guide::OptimizationTypeDecision::
           kHadHintButNotLoadedInTime;
     }
+
+    if (hints_fetcher_ &&
+        hints_fetcher_->IsHintForHostBeingFetched(url.host())) {
+      return optimization_guide::OptimizationTypeDecision::
+          kHintFetchStartedButNotAvailableInTime;
+    }
+
     return optimization_guide::OptimizationTypeDecision::kNoHintAvailable;
   }
 
@@ -1012,10 +1010,6 @@ void OptimizationGuideHintsManager::MaybeFetchHintsForNavigation(
     return;
   }
 
-  page_navigation_hosts_being_fetched_.clear();
-  page_navigation_hosts_being_fetched_.push_back(
-      navigation_handle->GetURL().host());
-
   if (!hints_fetcher_) {
     hints_fetcher_ = std::make_unique<optimization_guide::HintsFetcher>(
         url_loader_factory_,
@@ -1028,7 +1022,8 @@ void OptimizationGuideHintsManager::MaybeFetchHintsForNavigation(
       optimization_guide::proto::CONTEXT_PAGE_NAVIGATION,
       base::BindOnce(
           &OptimizationGuideHintsManager::OnPageNavigationHintsFetched,
-          ui_weak_ptr_factory_.GetWeakPtr()));
+          ui_weak_ptr_factory_.GetWeakPtr(),
+          base::flat_set<std::string>({navigation_handle->GetURL().host()})));
 
   if (!hosts.empty() && !urls.empty()) {
     race_navigation_recorder.set_race_attempt_status(
@@ -1042,10 +1037,3 @@ void OptimizationGuideHintsManager::ClearFetchedHints() {
   optimization_guide::HintsFetcher::ClearHostsSuccessfullyFetched(
       pref_service_);
 }
-
-bool OptimizationGuideHintsManager::IsHintBeingFetched(
-    const std::string& host) const {
-  return std::find(page_navigation_hosts_being_fetched_.begin(),
-                   page_navigation_hosts_being_fetched_.end(),
-                   host) != page_navigation_hosts_being_fetched_.end();
-}

chrome/browser/optimization_guide/optimization_guide_hints_manager.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_OPTIMIZATION_GUIDE_OPTIMIZATION_GUIDE_HINTS_MANAGER_H_
 
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "base/callback_forward.h"
@@ -217,8 +218,10 @@ class OptimizationGuideHintsManager
 
   // Called when the hints for a navigation have been fetched from the remote
   // Optimization Guide Service and are ready for parsing. This is used when
-  // fetching hints in real-time.
+  // fetching hints in real-time. |page_navigation_hosts_requested| contains the
+  // hosts that were requested to be fetched.
   void OnPageNavigationHintsFetched(
+      const base::flat_set<std::string>& page_navigation_hosts_requested,
       base::Optional<
           std::unique_ptr<optimization_guide::proto::GetHintsResponse>>
           get_hints_response);
@@ -229,7 +232,10 @@ class OptimizationGuideHintsManager
 
   // Called when the fetched hints have been stored in |hint_cache| and are
   // ready to be used. This is used when hints were fetched in real-time.
-  void OnFetchedPageNavigationHintsStored();
+  // |page_navigation_hosts_requested| contains the hosts whose hints should be
+  // loaded into memory when invoked.
+  void OnFetchedPageNavigationHintsStored(
+      const base::flat_set<std::string>& page_navigation_hosts_requested);
 
   // Returns the time when a hints fetch request was last attempted.
   base::Time GetLastHintsFetchAttemptTime() const;
@@ -267,10 +273,6 @@ class OptimizationGuideHintsManager
       const base::Optional<NavigationPredictorKeyedService::Prediction>&
           prediction) override;
 
-  // Returns whether a hint for |host| is currently being fetched from the
-  // remote Optimization Guide Service.
-  bool IsHintBeingFetched(const std::string& host) const;
-
   // Creates a hints fetch for |navigation_handle| if it is allowed. The
   // fetch will include the host and URL of the |navigation_handle| if the
   // associated hints for each are not already in the cache.
@@ -345,11 +347,6 @@ class OptimizationGuideHintsManager
   // Used in testing to subscribe to an update event in this class.
   base::OnceClosure next_update_closure_;
 
-  // Hosts for which hints are currently being fetched with the PAGE_NAVIGATION
-  // request context.
-  // This will be cleared at request completion.
-  std::vector<std::string> page_navigation_hosts_being_fetched_;
-
   // Used to get |weak_ptr_| to self on the UI thread.
   base::WeakPtrFactory<OptimizationGuideHintsManager> ui_weak_ptr_factory_{
       this};

chrome/browser/optimization_guide/optimization_guide_hints_manager_unittest.cc

@@ -192,11 +192,13 @@ class TestHintsFetcher : public optimization_guide::HintsFetcher {
         std::move(hints_fetched_callback).Run(base::nullopt);
         return false;
       case HintsFetcherEndState::kFetchSuccessWithHostHints:
+        hosts_fetched_ = hosts;
         hints_fetched_ = true;
         std::move(hints_fetched_callback)
             .Run(BuildHintsResponse({"host.com"}, {}));
         return true;
       case HintsFetcherEndState::kFetchSuccessWithURLHints:
+        hosts_fetched_ = hosts;
         hints_fetched_ = true;
         std::move(hints_fetched_callback)
             .Run(BuildHintsResponse({},
@@ -210,9 +212,15 @@ class TestHintsFetcher : public optimization_guide::HintsFetcher {
     return true;
   }
 
+  bool IsHintForHostBeingFetched(const std::string& host) const override {
+    return std::find(hosts_fetched_.begin(), hosts_fetched_.end(), host) !=
+           hosts_fetched_.end();
+  }
+
   bool hints_fetched() { return hints_fetched_; }
 
  private:
+  std::vector<std::string> hosts_fetched_;
   bool hints_fetched_ = false;
   HintsFetcherEndState fetch_state_;
 };
@@ -2543,7 +2551,7 @@ TEST_F(OptimizationGuideHintsManagerFetchingTest,
       CreateMockNavigationHandleWithOptimizationGuideWebContentsObserver(
           url_without_hints());
   hints_manager()->SetHintsFetcherForTesting(
-      BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithNoHints));
+      BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithHostHints));
   hints_manager()->OnNavigationStartOrRedirect(navigation_handle.get(),
                                                base::DoNothing());
   optimization_guide::OptimizationMetadata optimization_metadata;

components/optimization_guide/hints_fetcher.cc

@@ -250,6 +250,13 @@ bool HintsFetcher::FetchOptimizationGuideServiceHints(
   return true;
 }
 
+bool HintsFetcher::IsHintForHostBeingFetched(const std::string& host) const {
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  return std::find(hosts_fetched_.begin(), hosts_fetched_.end(), host) !=
+         hosts_fetched_.end();
+}
+
 void HintsFetcher::HandleResponse(const std::string& get_hints_response_data,
                                   int net_status,
                                   int response_code) {
@@ -286,6 +293,7 @@ void HintsFetcher::HandleResponse(const std::string& get_hints_response_data,
                                  HintsFetcherRequestStatus::kSuccess);
     std::move(hints_fetched_callback_).Run(std::move(get_hints_response));
   } else {
+    hosts_fetched_.clear();
     RecordRequestStatusHistogram(request_context_,
                                  HintsFetcherRequestStatus::kResponseError);
     std::move(hints_fetched_callback_).Run(base::nullopt);

components/optimization_guide/hints_fetcher.h

@@ -93,6 +93,10 @@ class HintsFetcher {
       optimization_guide::proto::RequestContext request_context,
       HintsFetchedCallback hints_fetched_callback);
 
+  // Returns whether a hint for the host is currently being fetched from the
+  // remote Optimization Guide Service. Virtualized for testing.
+  virtual bool IsHintForHostBeingFetched(const std::string& host) const;
+
   // Set |time_clock_| for testing.
   void SetTimeClockForTesting(const base::Clock* time_clock);
 

components/optimization_guide/hints_fetcher_unittest.cc

@@ -137,6 +137,10 @@ class HintsFetcherTest : public testing::Test {
                                                GetMockClock());
   }
 
+  bool IsHintForHostBeingFetched(const std::string& host) {
+    return hints_fetcher_->IsHintForHostBeingFetched(host);
+  }
+
  private:
   void RunUntilIdle() {
     task_environment_.RunUntilIdle();
@@ -288,8 +292,10 @@ TEST_F(HintsFetcherTest, FetchReturnBadResponse) {
   std::string response_content = "not proto";
   EXPECT_TRUE(FetchHints({"foo.com"}, {} /* urls */));
   VerifyHasPendingFetchRequests();
+  EXPECT_TRUE(IsHintForHostBeingFetched("foo.com"));
   EXPECT_TRUE(SimulateResponse(response_content, net::HTTP_OK));
   EXPECT_FALSE(hints_fetched());
+  EXPECT_FALSE(IsHintForHostBeingFetched("foo.com"));
 
   // Make sure histograms are recorded correctly on bad response.
   histogram_tester.ExpectTotalCount(
@@ -307,6 +313,7 @@ TEST_F(HintsFetcherTest, FetchAttemptWhenNetworkOffline) {
   std::string response_content;
   EXPECT_FALSE(FetchHints({"foo.com"}, {} /* urls */));
   EXPECT_FALSE(hints_fetched());
+  EXPECT_FALSE(IsHintForHostBeingFetched("foo.com"));
 
   // Make sure histograms are recorded correctly on bad response.
   histogram_tester.ExpectTotalCount(
@@ -335,8 +342,12 @@ TEST_F(HintsFetcherTest, HintsFetchSuccessfulHostsRecorded) {
 
   EXPECT_TRUE(FetchHints(hosts, {} /* urls */));
   VerifyHasPendingFetchRequests();
+  EXPECT_TRUE(IsHintForHostBeingFetched("host1.com"));
+  EXPECT_TRUE(IsHintForHostBeingFetched("host2.com"));
   EXPECT_TRUE(SimulateResponse(response_content, net::HTTP_OK));
   EXPECT_TRUE(hints_fetched());
+  EXPECT_FALSE(IsHintForHostBeingFetched("host1.com"));
+  EXPECT_FALSE(IsHintForHostBeingFetched("host2.com"));
 
   const base::DictionaryValue* hosts_fetched = pref_service()->GetDictionary(
       prefs::kHintsFetcherHostsSuccessfullyFetched);
@@ -566,8 +577,7 @@ TEST_F(HintsFetcherTest, NoHostsOrURLsToFetch) {
   base::HistogramTester histogram_tester;
   std::string response_content;
 
-  EXPECT_FALSE(FetchHints({} /* hosts */, {} /* urls */
-                          ));
+  EXPECT_FALSE(FetchHints({} /* hosts */, {} /* urls */));
   EXPECT_FALSE(hints_fetched());
   histogram_tester.ExpectUniqueSample(
       "OptimizationGuide.HintsFetcher.GetHintsRequest.RequestStatus."