Fix last committed URL in prediction manager

Use WebContents::GetLastCommittedURL to compute the last URL.

Change-Id: I829c01287a9902c302c87d7a127bde70303558a3
Bug: 1037945
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982235
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#727603}

chrome/browser/optimization_guide/optimization_guide_navigation_data.cc

@@ -58,7 +58,8 @@ OptimizationGuideNavigationData::OptimizationGuideNavigationData(
       was_host_covered_by_fetch_at_commit_(
           other.was_host_covered_by_fetch_at_commit_),
       was_hint_for_host_attempted_to_be_fetched_(
-          other.was_hint_for_host_attempted_to_be_fetched_) {
+          other.was_hint_for_host_attempted_to_be_fetched_),
+      is_same_origin_navigation_(other.is_same_origin_navigation_) {
   if (other.has_page_hint_value()) {
     page_hint_ = std::make_unique<optimization_guide::proto::PageHint>(
         *other.page_hint());

chrome/browser/optimization_guide/optimization_guide_navigation_data.h

@@ -139,6 +139,12 @@ class OptimizationGuideNavigationData {
         was_hint_for_host_attempted_to_be_fetched;
   }
 
+  // Whether the initiation of the navigation was from a same origin URL or not.
+  bool is_same_origin_navigation() const { return is_same_origin_navigation_; }
+  void set_is_same_origin_navigation(bool is_same_origin_navigation) {
+    is_same_origin_navigation_ = is_same_origin_navigation;
+  }
+
  private:
   // Records the hint cache and fetch coverage based on data currently held in
   // |this|.
@@ -210,6 +216,9 @@ class OptimizationGuideNavigationData {
   // during the navigation.
   base::Optional<bool> was_hint_for_host_attempted_to_be_fetched_;
 
+  // Whether the initiation of the navigation was from a same origin URL or not.
+  bool is_same_origin_navigation_ = false;
+
   DISALLOW_ASSIGN(OptimizationGuideNavigationData);
 };
 

chrome/browser/optimization_guide/optimization_guide_web_contents_observer.cc

@@ -85,6 +85,12 @@ void OptimizationGuideWebContentsObserver::DidStartNavigation(
   if (!navigation_handle->IsInMainFrame())
     return;
 
+  content::WebContents* web_contents = navigation_handle->GetWebContents();
+  bool is_same_origin =
+      web_contents &&
+      web_contents->GetLastCommittedURL().SchemeIsHTTPOrHTTPS() &&
+      url::IsSameOriginWith(navigation_handle->GetURL(),
+                            web_contents->GetLastCommittedURL());
   OptimizationGuideTopHostProvider::MaybeUpdateTopHostBlacklist(
       navigation_handle);
 
@@ -100,6 +106,7 @@ void OptimizationGuideWebContentsObserver::DidStartNavigation(
       GetOrCreateOptimizationGuideNavigationData(navigation_handle);
   nav_data->set_was_host_covered_by_fetch_at_navigation_start(
       was_host_covered_by_fetch);
+  nav_data->set_is_same_origin_navigation(is_same_origin);
 }
 
 void OptimizationGuideWebContentsObserver::DidRedirectNavigation(

chrome/browser/optimization_guide/prediction/prediction_manager.cc

@@ -10,6 +10,7 @@
 #include "base/containers/flat_set.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/metrics/histogram_macros_local.h"
 #include "base/rand_util.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/stringprintf.h"
@@ -288,8 +289,16 @@ base::Optional<float> PredictionManager::GetValueForClientFeature(
           engagement_service->GetScore(navigation_handle->GetURL()));
     }
     case proto::CLIENT_MODEL_FEATURE_SAME_ORIGIN_NAVIGATION: {
-      return static_cast<float>(url::IsSameOriginWith(
-          navigation_handle->GetURL(), navigation_handle->GetPreviousURL()));
+      OptimizationGuideNavigationData* nav_data =
+          OptimizationGuideNavigationData::GetFromNavigationHandle(
+              navigation_handle);
+
+      bool is_same_origin = nav_data && nav_data->is_same_origin_navigation();
+
+      LOCAL_HISTOGRAM_BOOLEAN(
+          "OptimizationGuide.PredictionManager.IsSameOrigin", is_same_origin);
+
+      return static_cast<float>(is_same_origin);
     }
     case proto::CLIENT_MODEL_FEATURE_FIRST_CONTENTFUL_PAINT_SESSION_MEAN: {
       if (session_fcp_.GetNumberOfSamples() == 0) {

chrome/browser/optimization_guide/prediction/prediction_manager_browsertest.cc

@@ -49,22 +49,20 @@ int GetTotalHistogramSamples(const base::HistogramTester* histogram_tester,
 }
 
 // Retries fetching |histogram_name| until it contains at least |count| samples.
-int RetryForHistogramUntilCountReached(
+void RetryForHistogramUntilCountReached(
     const base::HistogramTester* histogram_tester,
     const std::string& histogram_name,
     int count) {
-  int total = 0;
   while (true) {
     base::ThreadPoolInstance::Get()->FlushForTesting();
     base::RunLoop().RunUntilIdle();
 
-    total = GetTotalHistogramSamples(histogram_tester, histogram_name);
-    if (total >= count)
-      return total;
-
     content::FetchHistogramsFromChildProcesses();
     SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
-    base::RunLoop().RunUntilIdle();
+
+    int total = GetTotalHistogramSamples(histogram_tester, histogram_name);
+    if (total >= count)
+      return;
   }
 }
 
@@ -292,6 +290,12 @@ class PredictionManagerBrowserTest : public InProcessBrowserTest {
   GURL https_url_with_content() { return https_url_with_content_; }
   GURL https_url_without_content() { return https_url_without_content_; }
 
+ protected:
+  // Feature that the model server should return in response to
+  // GetModelsRequest.
+  proto::ClientModelFeature client_model_feature_ =
+      optimization_guide::proto::CLIENT_MODEL_FEATURE_SITE_ENGAGEMENT_SCORE;
+
  private:
   std::unique_ptr<net::test_server::HttpResponse> HandleGetModelsRequest(
       const net::test_server::HttpRequest& request) {
@@ -305,8 +309,7 @@ class PredictionManagerBrowserTest : public InProcessBrowserTest {
     std::unique_ptr<optimization_guide::proto::GetModelsResponse>
         get_models_response = BuildGetModelsResponse(
             {"example1.com", https_server_->GetURL("/").host()},
-            {optimization_guide::proto::
-                 CLIENT_MODEL_FEATURE_SITE_ENGAGEMENT_SCORE});
+            {client_model_feature_});
     if (response_type_ == PredictionModelsFetcherRemoteResponseType::
                               kSuccessfulWithFeaturesAndNoModels) {
       get_models_response->clear_models();
@@ -469,12 +472,9 @@ IN_PROC_BROWSER_TEST_F(
 
   // Wait until histograms have been updated before performing checks for
   // correct behavior based on the response.
-  EXPECT_GE(
-      RetryForHistogramUntilCountReached(
-          &histogram_tester,
-          "OptimizationGuide.PredictionModelFetcher.GetModelsResponse.Status",
-          1),
-      1);
+  RetryForHistogramUntilCountReached(
+      &histogram_tester,
+      "OptimizationGuide.PredictionModelFetcher.GetModelsResponse.Status", 1);
 
   histogram_tester.ExpectBucketCount(
       "OptimizationGuide.PredictionModelFetcher.GetModelsResponse.Status",
@@ -499,23 +499,17 @@ IN_PROC_BROWSER_TEST_F(
 
   // Wait until histograms have been updated before performing checks for
   // correct behavior based on the response.
-  EXPECT_GE(
-      RetryForHistogramUntilCountReached(
-          &histogram_tester,
-          "OptimizationGuide.PredictionModelFetcher.GetModelsResponse.Status",
-          1),
-      1);
+  RetryForHistogramUntilCountReached(
+      &histogram_tester,
+      "OptimizationGuide.PredictionModelFetcher.GetModelsResponse.Status", 1);
 
-  EXPECT_GE(
-      RetryForHistogramUntilCountReached(
-          &histogram_tester,
-          "OptimizationGuide.PredictionManager.HostModelFeaturesStored", 1),
-      1);
-  EXPECT_GE(
-      RetryForHistogramUntilCountReached(
-          &histogram_tester,
-          "OptimizationGuide.PredictionManager.PredictionModelsStored", 1),
-      1);
+  RetryForHistogramUntilCountReached(
+      &histogram_tester,
+      "OptimizationGuide.PredictionManager.HostModelFeaturesStored", 1);
+
+  RetryForHistogramUntilCountReached(
+      &histogram_tester,
+      "OptimizationGuide.PredictionManager.PredictionModelsStored", 1);
 
   ui_test_utils::NavigateToURL(browser(), https_url_with_content());
 
@@ -534,4 +528,70 @@ IN_PROC_BROWSER_TEST_F(
       1);
 }
 
+class PredictionManagerBrowserSameOriginTest
+    : public PredictionManagerBrowserTest {
+ public:
+  PredictionManagerBrowserSameOriginTest() = default;
+  ~PredictionManagerBrowserSameOriginTest() override = default;
+
+  void SetUp() override {
+    client_model_feature_ =
+        optimization_guide::proto::CLIENT_MODEL_FEATURE_SAME_ORIGIN_NAVIGATION;
+    PredictionManagerBrowserTest::SetUp();
+  }
+};
+
+// Regression test for https://crbug.com/1037945. Tests that the origin of the
+// previous navigation is computed correctly.
+IN_PROC_BROWSER_TEST_F(PredictionManagerBrowserSameOriginTest,
+                       DISABLE_ON_WIN_MAC_CHROMEOS(IsSameOriginNavigation)) {
+  base::HistogramTester histogram_tester;
+  OptimizationGuideKeyedService* keyed_service =
+      OptimizationGuideKeyedServiceFactory::GetForProfile(browser()->profile());
+
+  PredictionManager* prediction_manager = keyed_service->GetPredictionManager();
+  EXPECT_TRUE(prediction_manager);
+  RegisterWithKeyedService();
+
+  // Wait until histograms have been updated before performing checks for
+  // correct behavior based on the response.
+  RetryForHistogramUntilCountReached(
+      &histogram_tester,
+      "OptimizationGuide.PredictionModelFetcher.GetModelsResponse.Status", 1);
+
+  RetryForHistogramUntilCountReached(
+      &histogram_tester,
+      "OptimizationGuide.PredictionManager.HostModelFeaturesStored", 1);
+
+  RetryForHistogramUntilCountReached(
+      &histogram_tester,
+      "OptimizationGuide.PredictionManager.PredictionModelsStored", 1);
+
+  ui_test_utils::NavigateToURL(browser(), https_url_with_content());
+  RetryForHistogramUntilCountReached(
+      &histogram_tester, "OptimizationGuide.PredictionManager.IsSameOrigin", 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.PredictionManager.IsSameOrigin", false, 1);
+
+  // Navigate to the same URL in the same tab. This should count as a
+  // same-origin navigation.
+  ui_test_utils::NavigateToURL(browser(), https_url_with_content());
+  RetryForHistogramUntilCountReached(
+      &histogram_tester, "OptimizationGuide.PredictionManager.IsSameOrigin", 2);
+  histogram_tester.ExpectBucketCount(
+      "OptimizationGuide.PredictionManager.IsSameOrigin", false, 1);
+  histogram_tester.ExpectBucketCount(
+      "OptimizationGuide.PredictionManager.IsSameOrigin", true, 1);
+
+  // Navigate to a cross-origin URL. This should count as a cross-origin
+  // navigation.
+  ui_test_utils::NavigateToURL(browser(), GURL("https://www.google.com/"));
+  RetryForHistogramUntilCountReached(
+      &histogram_tester, "OptimizationGuide.PredictionManager.IsSameOrigin", 3);
+  histogram_tester.ExpectBucketCount(
+      "OptimizationGuide.PredictionManager.IsSameOrigin", false, 2);
+  histogram_tester.ExpectBucketCount(
+      "OptimizationGuide.PredictionManager.IsSameOrigin", true, 1);
+}
+
 }  // namespace optimization_guide

chrome/browser/optimization_guide/prediction/prediction_manager_unittest.cc

@@ -1425,12 +1425,6 @@ TEST_P(PredictionManagerTest, ClientFeature) {
   navigation_handle->set_url(previous_url);
   navigation_handle->set_page_transition(
       ui::PageTransition::PAGE_TRANSITION_RELOAD);
-  ON_CALL(*navigation_handle, GetPreviousURL())
-      .WillByDefault(testing::ReturnRef(previous_url));
-
-  if (IsSameOriginNavigationFeature()) {
-    EXPECT_CALL(*navigation_handle, GetPreviousURL()).Times(1);
-  }
 
   CreatePredictionManager({});
   prediction_manager()->SetPredictionModelFetcherForTesting(
@@ -1477,8 +1471,6 @@ TEST_F(PredictionManagerTest, PreviousSessionStatisticsUsed) {
   navigation_handle->set_url(previous_url);
   navigation_handle->set_page_transition(
       ui::PageTransition::PAGE_TRANSITION_RELOAD);
-  ON_CALL(*navigation_handle, GetPreviousURL())
-      .WillByDefault(testing::ReturnRef(previous_url));
 
   pref_service()->SetDouble(optimization_guide::prefs::kSessionStatisticFCPMean,
                             200.0);