Invoke callbacks for the whole redirect chain instead of final URL

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

chrome/browser/optimization_guide/optimization_guide_hints_manager.cc

@@ -1186,11 +1186,12 @@ void OptimizationGuideHintsManager::MaybeFetchHintsForNavigation(
 }
 
 void OptimizationGuideHintsManager::OnNavigationFinish(
-    const GURL& navigation_url,
+    const std::vector<GURL>& navigation_redirect_chain,
     OptimizationGuideNavigationData* navigation_data) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // Populate navigation data with hint information.
+  const GURL navigation_url = navigation_redirect_chain.back();
   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));
@@ -1205,10 +1206,12 @@ void OptimizationGuideHintsManager::OnNavigationFinish(
 
   // The callbacks will be invoked when the fetch request comes back, so it
   // will be cleaned up later.
-  if (IsHintBeingFetchedForNavigation(navigation_url))
-    return;
+  for (const auto& url : navigation_redirect_chain) {
+    if (IsHintBeingFetchedForNavigation(url))
+      continue;
 
-  PrepareToInvokeRegisteredCallbacks(navigation_url);
+    PrepareToInvokeRegisteredCallbacks(url);
+  }
 }
 
 void OptimizationGuideHintsManager::ClearFetchedHints() {

chrome/browser/optimization_guide/optimization_guide_hints_manager.h

@@ -156,9 +156,10 @@ class OptimizationGuideHintsManager
   void OnNavigationStartOrRedirect(content::NavigationHandle* navigation_handle,
                                    base::OnceClosure callback);
 
-  // Notifies |this| that a navigation with URL |navigation_url| has finished.
-  // The |navigation_data| will be updated based on the current state of |this|.
-  void OnNavigationFinish(const GURL& navigation_url,
+  // Notifies |this| that a navigation with redirect chain
+  // |navigation_redirect_chain| has finished. The |navigation_data| will be
+  // updated based on the current state of |this|.
+  void OnNavigationFinish(const std::vector<GURL>& navigation_redirect_chain,
                           OptimizationGuideNavigationData* navigation_data);
 
  private:

chrome/browser/optimization_guide/optimization_guide_hints_manager_unittest.cc

@@ -3077,6 +3077,43 @@ TEST_F(OptimizationGuideHintsManagerFetchingTest,
       optimization_guide::OptimizationTypeDecision::kAllowedByHint, 1);
 }
 
+TEST_F(OptimizationGuideHintsManagerFetchingTest,
+       CanApplyOptimizationAsyncDoesNotStrandCallbacksAtBeginningOfChain) {
+  base::HistogramTester histogram_tester;
+
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      optimization_guide::switches::kDisableCheckingUserPermissionsForTesting);
+  hints_manager()->RegisterOptimizationTypes(
+      {optimization_guide::proto::COMPRESS_PUBLIC_IMAGES});
+
+  InitializeWithDefaultConfig("1.0.0.0");
+
+  // Set ECT estimate so fetch is NOT activated.
+  hints_manager()->OnEffectiveConnectionTypeChanged(
+      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_4G);
+
+  GURL url_that_redirected("https://urlthatredirected.com");
+  std::unique_ptr<content::MockNavigationHandle> navigation_handle_redirect =
+      CreateMockNavigationHandleWithOptimizationGuideWebContentsObserver(
+          url_that_redirected);
+  hints_manager()->CanApplyOptimizationAsync(
+      url_that_redirected, optimization_guide::proto::COMPRESS_PUBLIC_IMAGES,
+      base::BindOnce(
+          [](optimization_guide::OptimizationGuideDecision decision,
+             const optimization_guide::OptimizationMetadata& metadata) {
+            EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
+                      decision);
+          }));
+  hints_manager()->OnNavigationFinish(
+      {url_that_redirected, GURL("https://otherurl.com/")},
+      /*navigation_data=*/nullptr);
+  RunUntilIdle();
+
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.ApplyDecisionAsync.CompressPublicImages",
+      optimization_guide::OptimizationTypeDecision::kNoHintAvailable, 1);
+}
+
 TEST_F(OptimizationGuideHintsManagerFetchingTest,
        CanApplyOptimizationAsyncDoesNotStrandCallbacksIfFetchNotPending) {
   base::HistogramTester histogram_tester;
@@ -3109,7 +3146,7 @@ TEST_F(OptimizationGuideHintsManagerFetchingTest,
             EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
                       decision);
           }));
-  hints_manager()->OnNavigationFinish(url_with_url_keyed_hint(),
+  hints_manager()->OnNavigationFinish({url_with_url_keyed_hint()},
                                       /*navigation_data=*/nullptr);
   RunUntilIdle();
 
@@ -3191,7 +3228,7 @@ TEST_F(OptimizationGuideHintsManagerFetchingTest,
                       decision);
             EXPECT_TRUE(metadata.public_image_metadata().has_value());
           }));
-  hints_manager()->OnNavigationFinish(url_with_url_keyed_hint(),
+  hints_manager()->OnNavigationFinish({url_with_url_keyed_hint()},
                                       /*navigation_data=*/nullptr);
   RunUntilIdle();
 
@@ -3215,7 +3252,7 @@ TEST_F(OptimizationGuideHintsManagerFetchingTest,
   OptimizationGuideNavigationData* navigation_data =
       OptimizationGuideNavigationData::GetFromNavigationHandle(
           navigation_handle.get());
-  hints_manager()->OnNavigationFinish(url_with_url_keyed_hint(),
+  hints_manager()->OnNavigationFinish({url_with_url_keyed_hint()},
                                       navigation_data);
 
   RunUntilIdle();

chrome/browser/optimization_guide/optimization_guide_keyed_service.cc

@@ -144,12 +144,14 @@ void OptimizationGuideKeyedService::OnNavigationStartOrRedirect(
 }
 
 void OptimizationGuideKeyedService::OnNavigationFinish(
-    const GURL& navigation_url,
+    const std::vector<GURL>& navigation_redirect_chain,
     OptimizationGuideNavigationData* navigation_data) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  if (hints_manager_)
-    hints_manager_->OnNavigationFinish(navigation_url, navigation_data);
+  if (hints_manager_) {
+    hints_manager_->OnNavigationFinish(navigation_redirect_chain,
+                                       navigation_data);
+  }
 }
 
 void OptimizationGuideKeyedService::RegisterOptimizationTypesAndTargets(

chrome/browser/optimization_guide/optimization_guide_keyed_service.h

@@ -73,8 +73,8 @@ class OptimizationGuideKeyedService
       content::NavigationHandle* navigation_handle);
 
   // Notifies |hints_manager_| that the navigation associated with
-  // |navigation_url| has finished.
-  void OnNavigationFinish(const GURL& navigation_url,
+  // |navigation_redirect_chain| has finished.
+  void OnNavigationFinish(const std::vector<GURL>& navigation_redirect_chain,
                           OptimizationGuideNavigationData* navigation_data);
 
   // Clears data specific to the user.

chrome/browser/optimization_guide/optimization_guide_keyed_service_browsertest.cc

@@ -30,6 +30,7 @@
 #include "components/previews/core/previews_switches.h"
 #include "components/ukm/test_ukm_recorder.h"
 #include "content/public/test/browser_test_utils.h"
+#include "net/base/escape.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
 #include "services/metrics/public/cpp/ukm_builders.h"
@@ -77,7 +78,18 @@ class OptimizationGuideConsumerWebContentsObserver
       : content::WebContentsObserver(web_contents) {}
   ~OptimizationGuideConsumerWebContentsObserver() override = default;
 
-  // contents::WebContentsObserver implementation:
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override {
+    if (callback_) {
+      OptimizationGuideKeyedService* service =
+          OptimizationGuideKeyedServiceFactory::GetForProfile(
+              Profile::FromBrowserContext(web_contents()->GetBrowserContext()));
+      service->CanApplyOptimizationAsync(navigation_handle,
+                                         optimization_guide::proto::NOSCRIPT,
+                                         std::move(callback_));
+    }
+  }
+
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override {
     OptimizationGuideKeyedService* service =
@@ -89,12 +101,6 @@ class OptimizationGuideConsumerWebContentsObserver
     last_can_apply_optimization_decision_ = service->CanApplyOptimization(
         navigation_handle, optimization_guide::proto::NOSCRIPT,
         /*optimization_metadata=*/nullptr);
-
-    if (callback_) {
-      service->CanApplyOptimizationAsync(navigation_handle,
-                                         optimization_guide::proto::NOSCRIPT,
-                                         std::move(callback_));
-    }
   }
 
   // Returns the last optimization guide decision that was returned by the
@@ -172,7 +178,10 @@ class OptimizationGuideKeyedServiceBrowserTest
 
     url_with_hints_ =
         https_server_->GetURL("somehost.com", "/hashints/whatever");
-    url_that_redirects_ = https_server_->GetURL("/redirect");
+    url_that_redirects_ =
+        https_server_->GetURL("/redirect?" + url_with_hints_.spec());
+    url_that_redirects_to_no_hints_ =
+        https_server_->GetURL("/redirect?https://nohints.com/");
 
     SetEffectiveConnectionType(
         net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_SLOW_2G);
@@ -245,23 +254,34 @@ class OptimizationGuideKeyedServiceBrowserTest
 
   GURL url_with_hints() { return url_with_hints_; }
 
-  GURL url_that_redirects() { return url_that_redirects_; }
+  GURL url_that_redirects_to_hints() { return url_that_redirects_; }
+
+  GURL url_that_redirects_to_no_hints() {
+    return url_that_redirects_to_no_hints_;
+  }
 
  private:
   std::unique_ptr<net::test_server::HttpResponse> HandleRequest(
       const net::test_server::HttpRequest& request) {
-    std::unique_ptr<net::test_server::BasicHttpResponse> response;
-    if (request.GetURL().spec().find("redirect") != std::string::npos) {
-      response.reset(new net::test_server::BasicHttpResponse);
-      response->set_code(net::HTTP_FOUND);
-      response->AddCustomHeader("Location", url_with_hints().spec());
+    if (request.GetURL().spec().find("redirect") == std::string::npos) {
+      return std::unique_ptr<net::test_server::HttpResponse>();
     }
-    return std::move(response);
+
+    GURL request_url = request.GetURL();
+    std::string dest =
+        net::UnescapeBinaryURLComponent(request_url.query_piece());
+
+    auto http_response =
+        std::make_unique<net::test_server::BasicHttpResponse>();
+    http_response->set_code(net::HTTP_FOUND);
+    http_response->AddCustomHeader("Location", dest);
+    return http_response;
   }
 
   std::unique_ptr<net::EmbeddedTestServer> https_server_;
   GURL url_with_hints_;
   GURL url_that_redirects_;
+  GURL url_that_redirects_to_no_hints_;
   base::test::ScopedFeatureList scoped_feature_list_;
   optimization_guide::testing::TestHintsComponentCreator
       test_hints_component_creator_;
@@ -296,6 +316,26 @@ IN_PROC_BROWSER_TEST_F(
   histogram_tester.ExpectTotalCount("OptimizationGuide.LoadedHint.Result", 0);
 }
 
+IN_PROC_BROWSER_TEST_F(OptimizationGuideKeyedServiceBrowserTest,
+                       NavigateToPageWithAsyncCallbackReturnsAnswerRedirect) {
+  PushHintsComponentAndWaitForCompletion();
+  RegisterWithKeyedService();
+
+  std::unique_ptr<base::RunLoop> run_loop = std::make_unique<base::RunLoop>();
+  SetCallbackOnConsumer(base::BindOnce(
+      [](base::RunLoop* run_loop,
+         optimization_guide::OptimizationGuideDecision decision,
+         const optimization_guide::OptimizationMetadata& metadata) {
+        EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
+                  decision);
+        run_loop->Quit();
+      },
+      run_loop.get()));
+
+  ui_test_utils::NavigateToURL(browser(), url_that_redirects_to_no_hints());
+  run_loop->Run();
+}
+
 IN_PROC_BROWSER_TEST_F(OptimizationGuideKeyedServiceBrowserTest,
                        NavigateToPageWithAsyncCallbackReturnsAnswer) {
   PushHintsComponentAndWaitForCompletion();
@@ -446,7 +486,7 @@ IN_PROC_BROWSER_TEST_F(
   ukm::TestAutoSetUkmRecorder ukm_recorder;
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), url_that_redirects());
+  ui_test_utils::NavigateToURL(browser(), url_that_redirects_to_hints());
 
   EXPECT_EQ(RetryForHistogramUntilCountReached(
                 histogram_tester, "OptimizationGuide.LoadedHint.Result", 2),

chrome/browser/optimization_guide/optimization_guide_web_contents_observer.cc

@@ -150,7 +150,7 @@ void OptimizationGuideWebContentsObserver::DidFinishNavigation(
                                     FlushMetricsAndNotifyNavigationFinish,
                                 weak_factory_.GetWeakPtr(),
                                 navigation_handle->GetNavigationId(),
-                                navigation_handle->GetURL(),
+                                navigation_handle->GetRedirectChain(),
                                 navigation_handle->HasCommitted()));
 
   if (!optimization_guide_keyed_service_)
@@ -163,19 +163,19 @@ void OptimizationGuideWebContentsObserver::DidFinishNavigation(
 }
 
 void OptimizationGuideWebContentsObserver::
-    FlushMetricsAndNotifyNavigationFinish(int64_t navigation_id,
-                                          const GURL& navigation_url,
-                                          bool has_committed) {
+    FlushMetricsAndNotifyNavigationFinish(
+        int64_t navigation_id,
+        const std::vector<GURL>& navigation_redirect_chain,
+        bool has_committed) {
   auto nav_data_iter =
       inflight_optimization_guide_navigation_datas_.find(navigation_id);
   if (nav_data_iter == inflight_optimization_guide_navigation_datas_.end())
     return;
 
-  // 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_)
+  if (optimization_guide_keyed_service_) {
     optimization_guide_keyed_service_->OnNavigationFinish(
-        navigation_url, &nav_data_iter->second);
+        navigation_redirect_chain, &nav_data_iter->second);
+  }
 
   (nav_data_iter->second).RecordMetrics(has_committed);
 

chrome/browser/optimization_guide/optimization_guide_web_contents_observer.h

@@ -61,9 +61,10 @@ class OptimizationGuideWebContentsObserver
   // |navigation_id| and then removes any data associated with it, including its
   // entry in |inflight_optimization_guide_navigation_datas_|. It also notifies
   // |optimization_guide_keyed_service_| that the navigation has finished.
-  void FlushMetricsAndNotifyNavigationFinish(int64_t navigation_id,
-                                             const GURL& navigation_url,
-                                             bool has_committed);
+  void FlushMetricsAndNotifyNavigationFinish(
+      int64_t navigation_id,
+      const std::vector<GURL>& navigation_redirect_chain,
+      bool has_committed);
 
   // The data related to a given navigation ID.
   std::map<int64_t, OptimizationGuideNavigationData>