Only prefetch the prefetch_url if the tab is in the foreground. Start a...

Only prefetch the prefetch_url if the tab is in the foreground. Start a prefetch when a backgrounded tab is switched to the foreground. Only prefetch any URL once.

Bug: 976071
Change-Id: If0a34ef5203794d34993c90dbe8db370b1780b2f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1667753
Commit-Queue: Sofiya Semenova <sofiyase@google.com>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#671467}

chrome/browser/navigation_predictor/navigation_predictor.cc

@@ -272,13 +272,18 @@ void NavigationPredictor::OnVisibilityChanged(content::Visibility visibility) {
   if (current_visibility_ == visibility)
     return;
 
-  // Check if the visibility changed from HIDDEN to VISIBLE. Since navigation
+  // Check if the visibility changed from VISIBLE to HIDDEN. Since navigation
   // predictor is currently restricted to Android, it is okay to disregard the
   // occluded state.
   if (current_visibility_ != content::Visibility::HIDDEN ||
       visibility != content::Visibility::VISIBLE) {
     current_visibility_ = visibility;
 
+    if (prerender_handle_) {
+      prerender_handle_->OnNavigateAway();
+      prerender_handle_.reset();
+    }
+
     // Stop any future preconnects while hidden.
     timer_.Stop();
     return;
@@ -289,6 +294,9 @@ void NavigationPredictor::OnVisibilityChanged(content::Visibility visibility) {
   // Previously, the visibility was HIDDEN, and now it is VISIBLE implying that
   // the web contents that was fully hidden is now fully visible.
   MaybePreconnectNow(Action::kPreconnectOnVisibilityChange);
+  if (prefetch_url_.has_value()) {
+    MaybePrefetch();
+  }
 }
 
 void NavigationPredictor::MaybePreconnectNow(Action log_action) {
@@ -790,9 +798,7 @@ void NavigationPredictor::MaybeTakeActionOnLoad(
   if (prefetch_url_.has_value()) {
     DCHECK_EQ(document_origin.host(), prefetch_url_->host());
     MaybePreconnectNow(Action::kPrefetch);
-    if (prefetch_after_preconnect_) {
-      Prerender(prefetch_url_.value());
-    }
+    MaybePrefetch();
     return;
   }
 
@@ -808,26 +814,38 @@ void NavigationPredictor::MaybeTakeActionOnLoad(
   base::UmaHistogramEnumeration(action_histogram_name, Action::kNone);
 }
 
-void NavigationPredictor::Prerender(const GURL& url) {
+void NavigationPredictor::MaybePrefetch() {
+  // If prefetches aren't allowed here, this URL has already
+  // been prefetched, or the current tab is hidden,
+  // we shouldn't prefetch again.
+  if (!prefetch_after_preconnect_ || prefetch_url_prefetched_ ||
+      current_visibility_ == content::Visibility::HIDDEN) {
+    return;
+  }
+
   prerender::PrerenderManager* prerender_manager =
       prerender::PrerenderManagerFactory::GetForBrowserContext(
           browser_context_);
 
   if (prerender_manager) {
-    content::SessionStorageNamespace* session_storage_namespace =
-        web_contents()->GetController().GetDefaultSessionStorageNamespace();
-    gfx::Size size = web_contents()->GetContainerBounds().size();
-
-    if (prerender_handle_) {
-      prerender_handle_->OnCancel();
-    }
-    // TODO(sofiyase): Only prerender when the tab is in the foreground.
-    // https://crbug.com/976071
-    prerender_handle_ = prerender_manager->AddPrerenderFromNavigationPredictor(
-        url, session_storage_namespace, size);
+    Prefetch(prerender_manager);
+    prefetch_url_prefetched_ = true;
   }
 }
 
+void NavigationPredictor::Prefetch(
+    prerender::PrerenderManager* prerender_manager) {
+  DCHECK(!prefetch_url_prefetched_);
+  DCHECK(!prerender_handle_);
+
+  content::SessionStorageNamespace* session_storage_namespace =
+      web_contents()->GetController().GetDefaultSessionStorageNamespace();
+  gfx::Size size = web_contents()->GetContainerBounds().size();
+
+  prerender_handle_ = prerender_manager->AddPrerenderFromNavigationPredictor(
+      prefetch_url_.value(), session_storage_namespace, size);
+}
+
 base::Optional<GURL> NavigationPredictor::GetUrlToPrefetch(
     const url::Origin& document_origin,
     const std::vector<std::unique_ptr<NavigationScore>>&

chrome/browser/navigation_predictor/navigation_predictor.h

@@ -27,6 +27,7 @@ class RenderFrameHost;
 
 namespace prerender {
 class PrerenderHandle;
+class PrerenderManager;
 }
 
 class SiteEngagementService;
@@ -100,6 +101,10 @@ class NavigationPredictor : public blink::mojom::AnchorElementMetricsHost,
   // URL that we decided to prefetch.
   base::Optional<GURL> prefetch_url_;
 
+  // True if a prefetch_url_ has already been prefetched, so we know
+  // whether to prefetch the url again when a tab becomes visible.
+  bool prefetch_url_prefetched_ = false;
+
  private:
   // Struct holding navigation score, rank and other info of the anchor element.
   // Used for look up when an anchor element is clicked.
@@ -149,8 +154,12 @@ class NavigationPredictor : public blink::mojom::AnchorElementMetricsHost,
       const std::vector<std::unique_ptr<NavigationScore>>&
           sorted_navigation_scores);
 
-  // Given a url to prerender, use PrerenderManager to prerender that page.
-  void Prerender(const GURL& url);
+  // Decides whether to prefetch a URL and, if yes, calls Prefetch.
+  void MaybePrefetch();
+
+  // Given a url to prefetch, uses PrerenderManager to start a NoStatePrefetch
+  // of that URL.
+  virtual void Prefetch(prerender::PrerenderManager* prerender_manager);
 
   base::Optional<GURL> GetUrlToPrefetch(
       const url::Origin& document_origin,

chrome/browser/navigation_predictor/navigation_predictor_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "content/public/browser/web_contents.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -46,6 +47,10 @@ class TestNavigationPredictor : public NavigationPredictor {
 
   const std::map<GURL, int>& GetAreaRankMap() const { return area_rank_map_; }
 
+  bool prefetch_url_prefetched() const { return prefetch_url_prefetched_; }
+
+  int calls_to_prefetch() const { return calls_to_prefetch_; }
+
  private:
   double CalculateAnchorNavigationScore(
       const blink::mojom::AnchorElementMetrics& metrics,
@@ -57,6 +62,10 @@ class TestNavigationPredictor : public NavigationPredictor {
     return 100 * metrics.ratio_area;
   }
 
+  void Prefetch(prerender::PrerenderManager* prerender_manager) override {
+    calls_to_prefetch_ += 1;
+  }
+
   // Maps from target URL to area rank of the anchor element.
   mutable std::map<GURL, int> area_rank_map_;
 
@@ -64,6 +73,8 @@ class TestNavigationPredictor : public NavigationPredictor {
 
   // Used to bind Mojo interface
   mojo::Binding<AnchorElementMetricsHost> binding_;
+
+  int calls_to_prefetch_ = 0;
 };
 
 class NavigationPredictorTest : public ChromeRenderViewHostTestHarness {
@@ -99,6 +110,10 @@ class NavigationPredictorTest : public ChromeRenderViewHostTestHarness {
     return predictor_service_helper_->preconnect_origin();
   }
 
+  bool prefetch_url_prefetched() {
+    return predictor_service_helper_->prefetch_url_prefetched();
+  }
+
  protected:
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
@@ -108,7 +123,8 @@ class NavigationPredictorTest : public ChromeRenderViewHostTestHarness {
   }
 
   void SetupFieldTrial(base::Optional<int> preconnect_origin_score_threshold,
-                       base::Optional<int> prefetch_url_score_threshold) {
+                       base::Optional<int> prefetch_url_score_threshold,
+                       base::Optional<bool> prefetch_after_preconnect) {
     if (field_trial_initiated_)
       return;
 
@@ -125,6 +141,10 @@ class NavigationPredictorTest : public ChromeRenderViewHostTestHarness {
       params["prefetch_url_score_threshold"] =
           base::NumberToString(prefetch_url_score_threshold.value());
     }
+    if (prefetch_after_preconnect.has_value()) {
+      params["prefetch_after_preconnect"] =
+          prefetch_after_preconnect.value() ? "true" : "false";
+    }
     scoped_feature_list.InitAndEnableFeatureWithParameters(
         blink::features::kNavigationPredictor, params);
   }
@@ -481,13 +501,57 @@ TEST_F(NavigationPredictorTest,
   EXPECT_FALSE(prefetch_url().has_value());
 }
 
+class NavigationPredictorPrefetchAfterPreconnectEnabledTest
+    : public NavigationPredictorTest {
+ public:
+  NavigationPredictorPrefetchAfterPreconnectEnabledTest() {
+    SetupFieldTrial(base::nullopt, base::nullopt, true);
+  }
+
+  void SetUp() override {
+    ChromeRenderViewHostTestHarness::SetUp();
+    predictor_service_helper_ = std::make_unique<TestNavigationPredictor>(
+        mojo::MakeRequest(&predictor_service_), main_rfh(), false);
+  }
+};
+
+// Test that a prefetch after preconnect occurs only when the current tab is
+// in the foreground, and that it does not occur multiple times for the same
+// URL.
+TEST_F(NavigationPredictorPrefetchAfterPreconnectEnabledTest,
+       PrefetchWithTabs) {
+  const std::string source = "https://example.com";
+  const std::string same_origin_href_large = "https://example.com/large";
+  std::vector<blink::mojom::AnchorElementMetricsPtr> metrics;
+  metrics.push_back(CreateMetricsPtr(source, same_origin_href_large, 1));
+
+  // Hide the tab and load the page. The URL should not be prefetched.
+  web_contents()->WasHidden();
+  predictor_service()->ReportAnchorElementMetricsOnLoad(std::move(metrics));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(prefetch_url_prefetched());
+  EXPECT_EQ(predictor_service_helper_->calls_to_prefetch(), 0);
+
+  // Making the tab visible should start a prefetch.
+  web_contents()->WasShown();
+  EXPECT_TRUE(prefetch_url_prefetched());
+  EXPECT_EQ(predictor_service_helper_->calls_to_prefetch(), 1);
+
+  // Switching a tab from HIDDEN to VISIBLE should not start a prefetch
+  // if a prefetch already occurred for that URL.
+  web_contents()->WasHidden();
+  web_contents()->WasShown();
+  EXPECT_TRUE(prefetch_url_prefetched());
+  EXPECT_EQ(predictor_service_helper_->calls_to_prefetch(), 1);
+}
+
 // Framework for testing cases where prefetch is effectively
 // disabled by setting |prefetch_url_score_threshold| to too high.
 class NavigationPredictorPrefetchDisabledTest : public NavigationPredictorTest {
  public:
   NavigationPredictorPrefetchDisabledTest() {
     SetupFieldTrial(0 /* preconnect_origin_score_threshold */,
-                    101 /* prefetch_url_score_threshold */);
+                    101 /* prefetch_url_score_threshold */, base::nullopt);
   }
 
   void SetUp() override {
@@ -583,7 +647,7 @@ class NavigationPredictorPreconnectPrefetchDisabledTest
  public:
   NavigationPredictorPreconnectPrefetchDisabledTest() {
     SetupFieldTrial(101 /* preconnect_origin_score_threshold */,
-                    101 /* prefetch_url_score_threshold */);
+                    101 /* prefetch_url_score_threshold */, base::nullopt);
   }
 
   void SetUp() override {