Refactor preconnect to its own WCO

This CL removes the preconnect logic from NavigationPredictor to
NavigationPredictorPreconnectClient. This will allow us to move from the
OnLoad event to a policy around the DidFinishNavigation event. The
rationale is that >10% of pages never reach OnLoad, so we may have a
better opportunity to preconnect from some delay after
DidFinishNavigation.

Change-Id: I7b8ee571acdc6a9c748c77ac9a7686e5b9c53b33
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1861106Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Robert Ogden <robertogden@chromium.org>
Commit-Queue: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707656}

chrome/browser/BUILD.gn

@@ -865,6 +865,8 @@ jumbo_static_library("browser") {
     "navigation_predictor/navigation_predictor_keyed_service.h",
     "navigation_predictor/navigation_predictor_keyed_service_factory.cc",
     "navigation_predictor/navigation_predictor_keyed_service_factory.h",
+    "navigation_predictor/navigation_predictor_preconnect_client.cc",
+    "navigation_predictor/navigation_predictor_preconnect_client.h",
     "net/chrome_cookie_notification_details.h",
     "net/chrome_mojo_proxy_resolver_factory.cc",
     "net/chrome_mojo_proxy_resolver_factory.h",

chrome/browser/navigation_predictor/navigation_predictor.h

@@ -13,7 +13,6 @@
 #include "base/optional.h"
 #include "base/sequence_checker.h"
 #include "base/time/time.h"
-#include "base/timer/timer.h"
 #include "content/public/browser/visibility.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
@@ -57,13 +56,13 @@ class NavigationPredictor : public blink::mojom::AnchorElementMetricsHost,
   enum class Action {
     kUnknown = 0,
     kNone = 1,
-    kPreresolve = 2,
-    kPreconnect = 3,
+    // DEPRECATED: kPreresolve = 2,
+    // DEPRECATED: kPreconnect = 3,
     kPrefetch = 4,
-    kPreconnectOnVisibilityChange = 5,
-    kDeprecatedPreconnectOnAppForeground = 6,  // Deprecated.
-    kPreconnectAfterTimeout = 7,
-    kMaxValue = kPreconnectAfterTimeout,
+    // DEPRECATED: kPreconnectOnVisibilityChange = 5,
+    // DEPRECATED: kPreconnectOnAppForeground = 6,  // Deprecated.
+    // DEPRECATED: kPreconnectAfterTimeout = 7,
+    kMaxValue = kPrefetch,
   };
 
   // Enum describing the accuracy of actions taken by the navigation predictor.
@@ -88,18 +87,15 @@ class NavigationPredictor : public blink::mojom::AnchorElementMetricsHost,
 
     // Navigation predictor preconnected to an origin, and an anchor element was
     // clicked whose URL had the same origin as the preconnected origin.
-    kPreconnectActionClickToSameOrigin = 4,
+    // DEPRECATED: kPreconnectActionClickToSameOrigin = 4,
 
     // Navigation predictor preconnected to an origin, and an anchor element was
     // clicked whose URL had a different origin than the preconnected origin.
-    kPreconnectActionClickToDifferentOrigin = 5,
-    kMaxValue = kPreconnectActionClickToDifferentOrigin,
+    // DEPRECATED: kPreconnectActionClickToDifferentOrigin = 5,
+    kMaxValue = kPrefetchActionClickToDifferentOrigin,
   };
 
  protected:
-  // Origin that we decided to preconnect to.
-  base::Optional<url::Origin> preconnect_origin_;
-
   // URL that we decided to prefetch.
   base::Optional<GURL> prefetch_url_;
 
@@ -166,11 +162,6 @@ class NavigationPredictor : public blink::mojom::AnchorElementMetricsHost,
       const std::vector<std::unique_ptr<NavigationScore>>&
           sorted_navigation_scores) const;
 
-  base::Optional<url::Origin> GetOriginToPreconnect(
-      const GURL& document_url,
-      const std::vector<std::unique_ptr<NavigationScore>>&
-          sorted_navigation_scores) const;
-
   // Record anchor element metrics on page load.
   void RecordMetricsOnLoad(
       const blink::mojom::AnchorElementMetrics& metric) const;
@@ -186,8 +177,8 @@ class NavigationPredictor : public blink::mojom::AnchorElementMetricsHost,
   // content::WebContentsObserver:
   void OnVisibilityChanged(content::Visibility visibility) override;
 
-  // MaybePreconnectNow preconnects to an origin server if it's allowed.
-  void MaybePreconnectNow(Action log_action);
+  // Records metrics on which action the predictor is taking.
+  void RecordAction(Action log_action);
 
   // Sends metrics to the UKM id at |ukm_source_id_|.
   void MaybeSendMetricsToUkm() const;
@@ -271,17 +262,8 @@ class NavigationPredictor : public blink::mojom::AnchorElementMetricsHost,
   // they are not comparable.
   const int prefetch_url_score_threshold_;
 
-  // Minimum preconnect score that the origin should have for preconnect. Note
-  // that scores of origins are computed differently from scores of URLs, so
-  // they are not comparable.
-  const int preconnect_origin_score_threshold_;
-
-  // True if |this| is allowed to preconnect to same origin hosts.
-  const bool same_origin_preconnecting_allowed_;
-
-  // True if |this| should use the PrerenderManager to prefetch after
-  // a preconnect.
-  const bool prefetch_after_preconnect_;
+  // True if |this| should use the PrerenderManager to prefetch.
+  const bool prefetch_enabled_;
 
   // True by default, otherwise navigation scores will not be normalized
   // by the sum of metrics weights nor normalized from 0 to 100 across
@@ -302,9 +284,6 @@ class NavigationPredictor : public blink::mojom::AnchorElementMetricsHost,
   // Current visibility state of the web contents.
   content::Visibility current_visibility_;
 
-  // Used to preconnect regularly.
-  base::OneShotTimer timer_;
-
   // PrerenderHandle returned after completing a prefetch in PrerenderManager.
   std::unique_ptr<prerender::PrerenderHandle> prerender_handle_;
 

chrome/browser/navigation_predictor/navigation_predictor_preconnect_client.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/navigation_predictor/navigation_predictor_preconnect_client.h"
+
+#include <memory>
+
+#include "base/bind.h"
+#include "base/metrics/field_trial_params.h"
+#include "base/time/time.h"
+#include "build/build_config.h"
+#include "chrome/browser/predictors/loading_predictor.h"
+#include "chrome/browser/predictors/loading_predictor_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "components/search_engines/template_url_service.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
+#include "net/base/features.h"
+
+namespace {
+
+// A holdback that prevents the preconnect to measure benefit of the feature.
+const base::Feature kNavigationPredictorPreconnectHoldback {
+  "NavigationPredictorPreconnectHoldback",
+#if defined(OS_ANDROID)
+      base::FEATURE_DISABLED_BY_DEFAULT
+#else
+      base::FEATURE_ENABLED_BY_DEFAULT
+#endif
+};
+
+}  // namespace
+
+NavigationPredictorPreconnectClient::NavigationPredictorPreconnectClient(
+    content::WebContents* web_contents)
+    : browser_context_(web_contents->GetBrowserContext()),
+      current_visibility_(web_contents->GetVisibility()) {}
+
+NavigationPredictorPreconnectClient::~NavigationPredictorPreconnectClient() =
+    default;
+
+void NavigationPredictorPreconnectClient::DidFinishNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (!navigation_handle->IsInMainFrame() ||
+      !navigation_handle->HasCommitted() || navigation_handle->IsSameDocument())
+    return;
+
+  // New page, so stop the preconnect timer.
+  timer_.Stop();
+}
+
+void NavigationPredictorPreconnectClient::OnVisibilityChanged(
+    content::Visibility visibility) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (current_visibility_ == visibility)
+    return;
+
+  // 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;
+
+    // Stop any future preconnects while hidden.
+    timer_.Stop();
+    return;
+  }
+
+  current_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();
+}
+
+void NavigationPredictorPreconnectClient::DidFinishLoad(
+    content::RenderFrameHost* render_frame_host,
+    const GURL& validated_url) {
+  // Ignore sub-frame loads.
+  if (render_frame_host->GetParent())
+    return;
+
+  MaybePreconnectNow();
+}
+
+void NavigationPredictorPreconnectClient::MaybePreconnectNow() {
+  if (base::FeatureList::IsEnabled(kNavigationPredictorPreconnectHoldback))
+    return;
+
+  if (browser_context_->IsOffTheRecord())
+    return;
+
+  // Only preconnect foreground tab.
+  if (current_visibility_ != content::Visibility::VISIBLE)
+    return;
+
+  // On search engine results page, next navigation is likely to be a different
+  // origin. Currently, the preconnect is only allowed for same origins. Hence,
+  // preconnect is currently disabled on search engine results page.
+  if (IsSearchEnginePage())
+    return;
+
+  url::Origin preconnect_origin =
+      url::Origin::Create(web_contents()->GetLastCommittedURL());
+  if (preconnect_origin.scheme() != url::kHttpScheme &&
+      preconnect_origin.scheme() != url::kHttpsScheme) {
+    return;
+  }
+
+  auto* loading_predictor = predictors::LoadingPredictorFactory::GetForProfile(
+      Profile::FromBrowserContext(browser_context_));
+  GURL preconnect_url_serialized(preconnect_origin.Serialize());
+  DCHECK(preconnect_url_serialized.is_valid());
+
+  loading_predictor->PrepareForPageLoad(
+      preconnect_url_serialized, predictors::HintOrigin::NAVIGATION_PREDICTOR,
+      true);
+
+  // The delay beyond the idle socket timeout that net uses when
+  // re-preconnecting. If negative, no retries occur.
+  constexpr int retry_delay_ms = 50;
+
+  // Set/Reset the timer to fire after the preconnect times out. Add an extra
+  // delay to make sure the preconnect has expired if it wasn't used.
+  timer_.Start(
+      FROM_HERE,
+      base::TimeDelta::FromSeconds(base::GetFieldTrialParamByFeatureAsInt(
+          net::features::kNetUnusedIdleSocketTimeout,
+          "unused_idle_socket_timeout_seconds", 60)) +
+          base::TimeDelta::FromMilliseconds(retry_delay_ms),
+      base::BindOnce(&NavigationPredictorPreconnectClient::MaybePreconnectNow,
+                     base::Unretained(this)));
+}
+
+bool NavigationPredictorPreconnectClient::IsSearchEnginePage() const {
+  auto* template_service = TemplateURLServiceFactory::GetForProfile(
+      Profile::FromBrowserContext(browser_context_));
+  if (!template_service)
+    return false;
+  return template_service->IsSearchResultsPageFromDefaultSearchProvider(
+      web_contents()->GetLastCommittedURL());
+}
+
+WEB_CONTENTS_USER_DATA_KEY_IMPL(NavigationPredictorPreconnectClient)

chrome/browser/navigation_predictor/navigation_predictor_preconnect_client.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_NAVIGATION_PREDICTOR_NAVIGATION_PREDICTOR_PRECONNECT_CLIENT_H_
+#define CHROME_BROWSER_NAVIGATION_PREDICTOR_NAVIGATION_PREDICTOR_PRECONNECT_CLIENT_H_
+
+#include "base/macros.h"
+#include "base/optional.h"
+#include "base/sequence_checker.h"
+#include "base/timer/timer.h"
+#include "content/public/browser/visibility.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_user_data.h"
+#include "url/origin.h"
+
+namespace content {
+class BrowserContext;
+class RenderFrameHost;
+}  // namespace content
+
+class NavigationPredictorPreconnectClient
+    : public content::WebContentsObserver,
+      public content::WebContentsUserData<NavigationPredictorPreconnectClient> {
+ public:
+  ~NavigationPredictorPreconnectClient() override;
+
+ private:
+  friend class content::WebContentsUserData<
+      NavigationPredictorPreconnectClient>;
+  explicit NavigationPredictorPreconnectClient(
+      content::WebContents* web_contents);
+
+  // content::WebContentsObserver:
+  void OnVisibilityChanged(content::Visibility visibility) override;
+  void DidFinishLoad(content::RenderFrameHost* render_frame_host,
+                     const GURL& validated_url) override;
+  void DidFinishNavigation(
+      content::NavigationHandle* navigation_handle) override;
+
+  // Returns template URL service. Guaranteed to be non-null.
+  bool IsSearchEnginePage() const;
+
+  base::Optional<url::Origin> GetOriginToPreconnect(
+      const GURL& document_url) const;
+
+  // MaybePreconnectNow preconnects to an origin server if it's allowed.
+  void MaybePreconnectNow();
+
+  // Used to get keyed services.
+  content::BrowserContext* const browser_context_;
+
+  // Current visibility state of the web contents.
+  content::Visibility current_visibility_;
+
+  // Used to preconnect regularly.
+  base::OneShotTimer timer_;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  WEB_CONTENTS_USER_DATA_KEY_DECL();
+
+  DISALLOW_COPY_AND_ASSIGN(NavigationPredictorPreconnectClient);
+};
+
+#endif  // CHROME_BROWSER_NAVIGATION_PREDICTOR_NAVIGATION_PREDICTOR_PRECONNECT_CLIENT_H_

chrome/browser/navigation_predictor/navigation_predictor_preconnect_client_browsertest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/macros.h"
+#include "base/run_loop.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/metrics/subprocess_metrics_provider.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/browser/subresource_filter/subresource_filter_browser_test_harness.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/search_test_utils.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "components/search_engines/template_url_service.h"
+#include "content/public/test/browser_test_utils.h"
+#include "net/base/features.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+
+namespace {
+
+class NavigationPredictorPreconnectClientBrowserTest
+    : public subresource_filter::SubresourceFilterBrowserTest {
+ public:
+  NavigationPredictorPreconnectClientBrowserTest()
+      : subresource_filter::SubresourceFilterBrowserTest() {
+    feature_list_.InitFromCommandLine(std::string(),
+                                      "NavigationPredictorPreconnectHoldback");
+  }
+
+  void SetUp() override {
+    https_server_.reset(
+        new net::EmbeddedTestServer(net::EmbeddedTestServer::TYPE_HTTPS));
+    https_server_->ServeFilesFromSourceDirectory(
+        "chrome/test/data/navigation_predictor");
+    ASSERT_TRUE(https_server_->Start());
+
+    subresource_filter::SubresourceFilterBrowserTest::SetUp();
+  }
+
+  void SetUpOnMainThread() override {
+    subresource_filter::SubresourceFilterBrowserTest::SetUpOnMainThread();
+    host_resolver()->ClearRules();
+  }
+
+  const GURL GetTestURL(const char* file) const {
+    return https_server_->GetURL(file);
+  }
+
+  // Retries fetching |histogram_name| until it contains at least |count|
+  // samples.
+  void RetryForHistogramUntilCountReached(
+      const base::HistogramTester& histogram_tester,
+      const std::string& histogram_name,
+      size_t count) {
+    base::RunLoop().RunUntilIdle();
+    for (size_t attempt = 0; attempt < 50; ++attempt) {
+      const std::vector<base::Bucket> buckets =
+          histogram_tester.GetAllSamples(histogram_name);
+      size_t total_count = 0;
+      for (const auto& bucket : buckets)
+        total_count += bucket.count;
+      if (total_count >= count)
+        return;
+      content::FetchHistogramsFromChildProcesses();
+      SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
+      base::RunLoop().RunUntilIdle();
+    }
+  }
+
+ private:
+  std::unique_ptr<net::EmbeddedTestServer> https_server_;
+  base::test::ScopedFeatureList feature_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(NavigationPredictorPreconnectClientBrowserTest);
+};
+
+IN_PROC_BROWSER_TEST_F(NavigationPredictorPreconnectClientBrowserTest,
+                       NoPreconnectSearch) {
+  static const char kShortName[] = "test";
+  static const char kSearchURL[] =
+      "/anchors_different_area.html?q={searchTerms}";
+  TemplateURLService* model =
+      TemplateURLServiceFactory::GetForProfile(browser()->profile());
+  ASSERT_TRUE(model);
+  search_test_utils::WaitForTemplateURLServiceToLoad(model);
+  ASSERT_TRUE(model->loaded());
+
+  TemplateURLData data;
+  data.SetShortName(base::ASCIIToUTF16(kShortName));
+  data.SetKeyword(data.short_name());
+  data.SetURL(GetTestURL(kSearchURL).spec());
+
+  TemplateURL* template_url = model->Add(std::make_unique<TemplateURL>(data));
+  ASSERT_TRUE(template_url);
+  model->SetUserSelectedDefaultSearchProvider(template_url);
+  const GURL& url = GetTestURL("/anchors_different_area.html?q=cats");
+
+  base::HistogramTester histogram_tester;
+  ui_test_utils::NavigateToURL(browser(), url);
+  // There should be preconnect from navigation, but not preconnect client.
+  histogram_tester.ExpectTotalCount("LoadingPredictor.PreconnectCount", 1);
+
+  ui_test_utils::NavigateToURL(browser(), url);
+  // There should be 2 preconnects from navigation, but not any from preconnect
+  // client.
+  histogram_tester.ExpectTotalCount("LoadingPredictor.PreconnectCount", 2);
+}
+
+IN_PROC_BROWSER_TEST_F(NavigationPredictorPreconnectClientBrowserTest,
+                       PreconnectNotSearch) {
+  const GURL& url = GetTestURL("/anchors_different_area.html");
+
+  base::HistogramTester histogram_tester;
+  ui_test_utils::NavigateToURL(browser(), url);
+  // There should be preconnect from navigation and one from preconnect client.
+  RetryForHistogramUntilCountReached(histogram_tester,
+                                     "LoadingPredictor.PreconnectCount", 2);
+}
+
+IN_PROC_BROWSER_TEST_F(NavigationPredictorPreconnectClientBrowserTest,
+                       PreconnectNotSearchBackgroundForeground) {
+  const GURL& url = GetTestURL("/anchors_different_area.html");
+
+  browser()->tab_strip_model()->GetActiveWebContents()->WasHidden();
+  base::HistogramTester histogram_tester;
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // There should be a navigational preconnect.
+  histogram_tester.ExpectTotalCount("LoadingPredictor.PreconnectCount", 1);
+
+  // Change to visible.
+  browser()->tab_strip_model()->GetActiveWebContents()->WasShown();
+
+  // After showing the contents, there should be a preconnect client preconnect.
+  RetryForHistogramUntilCountReached(histogram_tester,
+                                     "LoadingPredictor.PreconnectCount", 2);
+
+  browser()->tab_strip_model()->GetActiveWebContents()->WasHidden();
+
+  browser()->tab_strip_model()->GetActiveWebContents()->WasShown();
+  // After showing the contents again, there should be another preconnect client
+  // preconnect.
+  RetryForHistogramUntilCountReached(histogram_tester,
+                                     "LoadingPredictor.PreconnectCount", 3);
+}
+
+class NavigationPredictorPreconnectClientBrowserTestWithUnusedIdleSocketTimeout
+    : public NavigationPredictorPreconnectClientBrowserTest {
+ public:
+  NavigationPredictorPreconnectClientBrowserTestWithUnusedIdleSocketTimeout()
+      : NavigationPredictorPreconnectClientBrowserTest() {
+    feature_list_.InitAndEnableFeatureWithParameters(
+        net::features::kNetUnusedIdleSocketTimeout,
+        {{"unused_idle_socket_timeout_seconds", "0"}});
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+// Test that we preconnect after the last preconnect timed out.
+IN_PROC_BROWSER_TEST_F(
+    NavigationPredictorPreconnectClientBrowserTestWithUnusedIdleSocketTimeout,
+    ActionAccuracy_timeout) {
+  base::HistogramTester histogram_tester;
+
+  const GURL& url = GetTestURL("/page_with_same_host_anchor_element.html");
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  histogram_tester.ExpectTotalCount("LoadingPredictor.PreconnectCount", 1);
+}
+
+class NavigationPredictorPreconnectClientBrowserTestWithHoldback
+    : public NavigationPredictorPreconnectClientBrowserTest {
+ public:
+  NavigationPredictorPreconnectClientBrowserTestWithHoldback()
+      : NavigationPredictorPreconnectClientBrowserTest() {
+    feature_list_.InitFromCommandLine("NavigationPredictorPreconnectHoldback",
+                                      std::string());
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(
+    NavigationPredictorPreconnectClientBrowserTestWithHoldback,
+    NoPreconnectHoldback) {
+  const GURL& url = GetTestURL("/anchors_different_area.html");
+
+  base::HistogramTester histogram_tester;
+  ui_test_utils::NavigateToURL(browser(), url);
+  // There should be preconnect from navigation, but not preconnect client.
+  histogram_tester.ExpectTotalCount("LoadingPredictor.PreconnectCount", 1);
+
+  ui_test_utils::NavigateToURL(browser(), url);
+  // There should be 2 preconnects from navigation, but not any from preconnect
+  // client.
+  histogram_tester.ExpectTotalCount("LoadingPredictor.PreconnectCount", 2);
+}
+
+}  // namespace

chrome/browser/navigation_predictor/navigation_predictor_unittest.cc

@@ -46,10 +46,6 @@ class TestNavigationPredictor : public NavigationPredictor {
 
   base::Optional<GURL> prefetch_url() const { return prefetch_url_; }
 
-  base::Optional<url::Origin> preconnect_origin() const {
-    return preconnect_origin_;
-  }
-
   const std::map<GURL, int>& GetAreaRankMap() const { return area_rank_map_; }
 
   bool prefetch_url_prefetched() const { return prefetch_url_prefetched_; }
@@ -131,10 +127,6 @@ class NavigationPredictorTest : public ChromeRenderViewHostTestHarness {
     return predictor_service_helper_->prefetch_url();
   }
 
-  base::Optional<url::Origin> preconnect_origin() const {
-    return predictor_service_helper_->preconnect_origin();
-  }
-
   bool prefetch_url_prefetched() {
     return predictor_service_helper_->prefetch_url_prefetched();
   }
@@ -147,8 +139,7 @@ class NavigationPredictorTest : public ChromeRenderViewHostTestHarness {
         !field_trial_initiated_);
   }
 
-  void SetupFieldTrial(base::Optional<int> preconnect_origin_score_threshold,
-                       base::Optional<int> prefetch_url_score_threshold,
+  void SetupFieldTrial(base::Optional<int> prefetch_url_score_threshold,
                        base::Optional<bool> prefetch_after_preconnect) {
     if (field_trial_initiated_)
       return;
@@ -158,10 +149,6 @@ class NavigationPredictorTest : public ChromeRenderViewHostTestHarness {
     const std::string kGroupName = "GroupFoo2";  // Value not used
 
     std::map<std::string, std::string> params;
-    if (preconnect_origin_score_threshold.has_value()) {
-      params["preconnect_origin_score_threshold"] =
-          base::NumberToString(preconnect_origin_score_threshold.value());
-    }
     if (prefetch_url_score_threshold.has_value()) {
       params["prefetch_url_score_threshold"] =
           base::NumberToString(prefetch_url_score_threshold.value());
@@ -478,7 +465,7 @@ TEST_F(NavigationPredictorTest, ActionTaken_SameOrigin_Prefetch_NotSameOrigin) {
 
   histogram_tester.ExpectUniqueSample(
       "NavigationPredictor.OnNonDSE.ActionTaken",
-      NavigationPredictor::Action::kPreconnect, 1);
+      NavigationPredictor::Action::kNone, 1);
   EXPECT_FALSE(prefetch_url().has_value());
 
   auto metrics_clicked = CreateMetricsPtr(source, same_origin_href_small, 0.01);
@@ -488,8 +475,7 @@ TEST_F(NavigationPredictorTest, ActionTaken_SameOrigin_Prefetch_NotSameOrigin) {
 
   histogram_tester.ExpectUniqueSample(
       "NavigationPredictor.OnNonDSE.AccuracyActionTaken",
-      NavigationPredictor::ActionAccuracy::kPreconnectActionClickToSameOrigin,
-      1);
+      NavigationPredictor::ActionAccuracy::kNoActionTakenClickHappened, 1);
 }
 
 TEST_F(NavigationPredictorTest,
@@ -509,7 +495,7 @@ TEST_F(NavigationPredictorTest,
 
   histogram_tester.ExpectUniqueSample(
       "NavigationPredictor.OnNonDSE.ActionTaken",
-      NavigationPredictor::Action::kPreconnect, 1);
+      NavigationPredictor::Action::kNone, 1);
   EXPECT_FALSE(prefetch_url().has_value());
 }
 
@@ -517,7 +503,7 @@ class NavigationPredictorSendUkmMetricsEnabledTest
     : public NavigationPredictorTest {
  public:
   NavigationPredictorSendUkmMetricsEnabledTest() {
-    SetupFieldTrial(base::nullopt, base::nullopt, base::nullopt);
+    SetupFieldTrial(base::nullopt, base::nullopt);
   }
 
   void SetUp() override {
@@ -751,7 +737,7 @@ class NavigationPredictorPrefetchAfterPreconnectEnabledTest
     : public NavigationPredictorTest {
  public:
   NavigationPredictorPrefetchAfterPreconnectEnabledTest() {
-    SetupFieldTrial(base::nullopt, base::nullopt, true);
+    SetupFieldTrial(base::nullopt, true);
   }
 
   void SetUp() override {
@@ -811,8 +797,7 @@ TEST_F(NavigationPredictorPrefetchAfterPreconnectEnabledTest,
 class NavigationPredictorPrefetchDisabledTest : public NavigationPredictorTest {
  public:
   NavigationPredictorPrefetchDisabledTest() {
-    SetupFieldTrial(0 /* preconnect_origin_score_threshold */,
-                    101 /* prefetch_url_score_threshold */, base::nullopt);
+    SetupFieldTrial(101 /* prefetch_url_score_threshold */, base::nullopt);
   }
 
   void SetUp() override {
@@ -847,9 +832,8 @@ TEST_F(NavigationPredictorPrefetchDisabledTest,
 
   histogram_tester.ExpectUniqueSample(
       "NavigationPredictor.OnNonDSE.ActionTaken",
-      NavigationPredictor::Action::kPreconnect, 1);
+      NavigationPredictor::Action::kNone, 1);
   EXPECT_FALSE(prefetch_url().has_value());
-  EXPECT_EQ(url::Origin::Create(GURL(source)), preconnect_origin());
 
   auto metrics_clicked = CreateMetricsPtr(source, same_origin_href_small, 0.01);
   predictor_service()->ReportAnchorElementMetricsOnClick(
@@ -858,8 +842,7 @@ TEST_F(NavigationPredictorPrefetchDisabledTest,
 
   histogram_tester.ExpectUniqueSample(
       "NavigationPredictor.OnNonDSE.AccuracyActionTaken",
-      NavigationPredictor::ActionAccuracy::kPreconnectActionClickToSameOrigin,
-      1);
+      NavigationPredictor::ActionAccuracy::kNoActionTakenClickHappened, 1);
 }
 
 // Disables prefetch and loads a page where the preconnect score of a cross
@@ -886,9 +869,8 @@ TEST_F(NavigationPredictorPrefetchDisabledTest,
 
   histogram_tester.ExpectUniqueSample(
       "NavigationPredictor.OnNonDSE.ActionTaken",
-      NavigationPredictor::Action::kPreconnect, 1);
+      NavigationPredictor::Action::kNone, 1);
   EXPECT_FALSE(prefetch_url().has_value());
-  EXPECT_TRUE(preconnect_origin().has_value());
 
   auto metrics_clicked = CreateMetricsPtr(source, same_origin_href_small, 0.01);
   predictor_service()->ReportAnchorElementMetricsOnClick(
@@ -897,8 +879,7 @@ TEST_F(NavigationPredictorPrefetchDisabledTest,
 
   histogram_tester.ExpectUniqueSample(
       "NavigationPredictor.OnNonDSE.AccuracyActionTaken",
-      NavigationPredictor::ActionAccuracy::kPreconnectActionClickToSameOrigin,
-      1);
+      NavigationPredictor::ActionAccuracy::kNoActionTakenClickHappened, 1);
 }
 
 // Framework for testing cases where preconnect and prefetch are effectively
@@ -907,8 +888,7 @@ class NavigationPredictorPreconnectPrefetchDisabledTest
     : public NavigationPredictorTest {
  public:
   NavigationPredictorPreconnectPrefetchDisabledTest() {
-    SetupFieldTrial(101 /* preconnect_origin_score_threshold */,
-                    101 /* prefetch_url_score_threshold */, base::nullopt);
+    SetupFieldTrial(101 /* prefetch_url_score_threshold */, base::nullopt);
   }
 
   void SetUp() override {
@@ -938,7 +918,7 @@ TEST_F(NavigationPredictorPreconnectPrefetchDisabledTest,
 
   histogram_tester.ExpectUniqueSample(
       "NavigationPredictor.OnNonDSE.ActionTaken",
-      NavigationPredictor::Action::kPreconnect, 1);
+      NavigationPredictor::Action::kNone, 1);
   EXPECT_FALSE(prefetch_url().has_value());
 
   auto metrics_clicked = CreateMetricsPtr(source, same_origin_href_small, 0.01);
@@ -948,6 +928,5 @@ TEST_F(NavigationPredictorPreconnectPrefetchDisabledTest,
 
   histogram_tester.ExpectUniqueSample(
       "NavigationPredictor.OnNonDSE.AccuracyActionTaken",
-      NavigationPredictor::ActionAccuracy::kPreconnectActionClickToSameOrigin,
-      1);
+      NavigationPredictor::ActionAccuracy::kNoActionTakenClickHappened, 1);
 }

chrome/browser/ui/tab_helpers.cc

@@ -34,6 +34,7 @@
 #include "chrome/browser/metrics/oom/out_of_memory_reporter.h"
 #include "chrome/browser/metrics/renderer_uptime_web_contents_observer.h"
 #include "chrome/browser/native_file_system/native_file_system_permission_request_manager.h"
+#include "chrome/browser/navigation_predictor/navigation_predictor_preconnect_client.h"
 #include "chrome/browser/net/net_error_tab_helper.h"
 #include "chrome/browser/optimization_guide/optimization_guide_web_contents_observer.h"
 #include "chrome/browser/page_load_metrics/page_load_metrics_initialize.h"
@@ -232,6 +233,7 @@ void TabHelpers::AttachTabHelpers(WebContents* web_contents) {
   NativeFileSystemPermissionRequestManager::CreateForWebContents(web_contents);
   NavigationCorrectionTabObserver::CreateForWebContents(web_contents);
   NavigationMetricsRecorder::CreateForWebContents(web_contents);
+  NavigationPredictorPreconnectClient::CreateForWebContents(web_contents);
   OptimizationGuideWebContentsObserver::CreateForWebContents(web_contents);
   OutOfMemoryReporter::CreateForWebContents(web_contents);
   chrome::InitializePageLoadMetricsForWebContents(web_contents);

chrome/test/BUILD.gn

@@ -977,6 +977,7 @@ if (!is_android) {
       "../browser/metrics/ukm_browsertest.cc",
       "../browser/metrics/variations/force_field_trials_browsertest.cc",
       "../browser/navigation_predictor/navigation_predictor_browsertest.cc",
+      "../browser/navigation_predictor/navigation_predictor_preconnect_client_browsertest.cc",
       "../browser/net/chrome_accept_header_browsertest.cc",
       "../browser/net/chrome_mojo_proxy_resolver_factory_browsertest.cc",
       "../browser/net/chrome_network_delegate_browsertest.cc",