Revert "Predictor: Dispatch all predictions async from the resource throttle"

This reverts commit 24e730d2.

Reason for revert: The delay navigation throttle will ruin data here.
Let me re-land tomorrow.

Original change's description:
> Predictor: Dispatch all predictions async from the resource throttle
> 
> This is a small refactor that should have some performance wins. Now
> that PlzNavigate has launched, we should be dispatching our preconnects
> after the initial navigation request has gone out, not before. This is
> because frequently (especially on Android), dispatching preconnects
> tends to hog the IO thread, delaying the initial request.
> 
> Note that this predictor is probably going away, but this CPU work
> is in the critical path of some other work I am planning on tackling.
> 
> Bug: 792524
> Change-Id: I87e3c235be0480d766e3e45c9a97c4a26b15f60d
> Reviewed-on: https://chromium-review.googlesource.com/821090
> Commit-Queue: Charlie Harrison <csharrison@chromium.org>
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#523473}

TBR=mmenke@chromium.org,csharrison@chromium.org

Change-Id: I596b40d4d3f527de7e0df6c00f376281a2dbe381
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 792524
Reviewed-on: https://chromium-review.googlesource.com/823110Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523486}

chrome/browser/loader/predictor_resource_throttle.cc

@@ -5,9 +5,6 @@
 #include "chrome/browser/loader/predictor_resource_throttle.h"
 
 #include "base/memory/ptr_util.h"
-#include "base/sequenced_task_runner.h"
-#include "base/threading/sequenced_task_runner_handle.h"
-#include "base/trace_event/trace_event.h"
 #include "chrome/browser/net/predictor.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "content/public/browser/resource_request_info.h"
@@ -30,7 +27,7 @@ bool IsNavigationRequest(net::URLRequest* request) {
 PredictorResourceThrottle::PredictorResourceThrottle(
     net::URLRequest* request,
     chrome_browser_net::Predictor* predictor)
-    : request_(request), predictor_(predictor), weak_factory_(this) {}
+    : request_(request), predictor_(predictor) {}
 
 PredictorResourceThrottle::~PredictorResourceThrottle() {}
 
@@ -71,6 +68,8 @@ void PredictorResourceThrottle::WillStartRequest(bool* defer) {
     predictor_->LearnFromNavigation(referring_scheme_host, request_scheme_host);
   }
 
+  // Subresources for main frames are predicted when navigation starts, in
+  // PredictorTabHelper, so only handle predictions for subframes here.
   // If the referring host is equal to the request host, then the predictor has
   // already made any/all predictions when navigating to the referring host.
   // Don't update the RecentlySeen() time because the timed cache is already
@@ -78,7 +77,10 @@ void PredictorResourceThrottle::WillStartRequest(bool* defer) {
   if (IsNavigationRequest(request_) &&
       referring_scheme_host != request_scheme_host) {
     predictor_->timed_cache()->SetRecentlySeen(request_scheme_host);
-    DispatchPredictions(request_scheme_host, request_->site_for_cookies());
+    if (resource_type == content::RESOURCE_TYPE_SUB_FRAME) {
+      predictor_->PredictFrameSubresources(request_scheme_host,
+                                           request_->site_for_cookies());
+    }
   }
 }
 
@@ -113,26 +115,10 @@ void PredictorResourceThrottle::WillRedirectRequest(
   }
 
   predictor_->timed_cache()->SetRecentlySeen(new_scheme_host);
-  DispatchPredictions(new_scheme_host, redirect_info.new_site_for_cookies);
+  predictor_->PredictFrameSubresources(new_scheme_host,
+                                       redirect_info.new_site_for_cookies);
 }
 
 const char* PredictorResourceThrottle::GetNameForLogging() const {
   return "PredictorResourceThrottle";
 }
-
-void PredictorResourceThrottle::DispatchPredictions(
-    const GURL& url,
-    const GURL& site_for_cookies) {
-  // Dispatch predictions asynchronously to avoid blocking the actual request
-  // from going out to the network.
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&PredictorResourceThrottle::DoPredict,
-                     weak_factory_.GetWeakPtr(), url, site_for_cookies));
-}
-
-void PredictorResourceThrottle::DoPredict(const GURL& url,
-                                          const GURL& site_for_cookies) {
-  TRACE_EVENT0("loading", "PredictorResourceThrottle::DoPredict");
-  predictor_->PredictFrameSubresources(url, site_for_cookies);
-}

chrome/browser/loader/predictor_resource_throttle.h

@@ -8,7 +8,6 @@
 #include <memory>
 
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
 #include "content/public/browser/resource_throttle.h"
 
 namespace chrome_browser_net {
@@ -20,19 +19,20 @@ struct RedirectInfo;
 class URLRequest;
 }
 
-class GURL;
 class ProfileIOData;
 
 // This resource throttle tracks requests in order to help the predictor learn
 // resource relationships. It notifies the predictor of redirect and referrer
 // relationships, and populates the predictor's timed cache of ongoing
-// navigations. It also initiates predictive actions based on navigation
+// navigations. It also initiates predictive actions based on subframe
 // requests and redirects.
 // Note: This class does not issue predictive actions off of the initial main
 // frame request (before any redirects). That is done on the UI thread in
 // response to navigation callbacks in predictor_tab_helper.cc.
 // TODO(csharrison): This class shouldn't depend on chrome_browser_net. The
 // predictor should probably be moved here (along with its dependencies).
+// TODO(csharrison): When the PreconnectMore experiment is over, move all
+// prediction dispatching to the tab helper.
 class PredictorResourceThrottle : public content::ResourceThrottle {
  public:
   PredictorResourceThrottle(net::URLRequest* request,
@@ -50,14 +50,9 @@ class PredictorResourceThrottle : public content::ResourceThrottle {
   const char* GetNameForLogging() const override;
 
  private:
-  void DispatchPredictions(const GURL& url, const GURL& site_for_cookies);
-  void DoPredict(const GURL& url, const GURL& site_for_cookies);
-
   net::URLRequest* request_;
   chrome_browser_net::Predictor* predictor_;
 
-  base::WeakPtrFactory<PredictorResourceThrottle> weak_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(PredictorResourceThrottle);
 };
 

chrome/browser/net/predictor_browsertest.cc

@@ -61,7 +61,6 @@
 #include "net/url_request/url_request_test_job.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "url/gurl.h"
-#include "url/url_constants.h"
 
 using content::BrowserThread;
 using testing::HasSubstr;
@@ -1411,14 +1410,9 @@ IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
   EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
 
   // Now, navigate using a renderer initiated navigation and expect preconnects.
-  // Need to navigate to about:blank first so the referring site is different
-  // from the request site.
-  ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));
   EXPECT_TRUE(content::ExecuteScript(
       browser()->tab_strip_model()->GetActiveWebContents(),
-      base::StringPrintf(
-          "window.location.href='%s'",
-          embedded_test_server()->GetURL("/title1.html").spec().c_str())));
+      "window.location.href='/title1.html'"));
 
   // The renderer initiated navigation is not synchronous, so just wait for the
   // preconnects to go through.

chrome/browser/net/predictor_tab_helper.cc

@@ -6,6 +6,10 @@
 
 #include "chrome/browser/net/predictor.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_features.h"
+#include "chrome/common/url_constants.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/common/browser_side_navigation_policy.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
@@ -16,9 +20,41 @@ DEFINE_WEB_CONTENTS_USER_DATA_KEY(chrome_browser_net::PredictorTabHelper);
 namespace chrome_browser_net {
 
 PredictorTabHelper::PredictorTabHelper(content::WebContents* web_contents)
-    : content::WebContentsObserver(web_contents) {}
+    : content::WebContentsObserver(web_contents),
+      predicted_from_pending_entry_(false) {
+}
+
+PredictorTabHelper::~PredictorTabHelper() {
+}
 
-PredictorTabHelper::~PredictorTabHelper() {}
+void PredictorTabHelper::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (!base::FeatureList::IsEnabled(features::kPreconnectMore) &&
+      (!content::IsBrowserSideNavigationEnabled() ||
+       navigation_handle->IsRendererInitiated()))
+    return;
+  // Subframe navigations are handled in WitnessURLRequest.
+  if (!navigation_handle->IsInMainFrame())
+    return;
+  if (predicted_from_pending_entry_) {
+    predicted_from_pending_entry_ = false;
+    return;
+  }
+  PreconnectUrl(navigation_handle->GetURL());
+}
+
+void PredictorTabHelper::DidStartNavigationToPendingEntry(
+    const GURL& url,
+    content::ReloadType reload_type) {
+  // This method isn't needed with PlzNavigate (see comment in header for
+  // predicted_from_pending_entry_)
+  if (content::IsBrowserSideNavigationEnabled())
+    return;
+
+  // The standard way to preconnect based on navigation.
+  PreconnectUrl(url);
+  predicted_from_pending_entry_ = true;
+}
 
 void PredictorTabHelper::DocumentOnLoadCompletedInMainFrame() {
   Profile* profile =
@@ -32,4 +68,12 @@ void PredictorTabHelper::DocumentOnLoadCompletedInMainFrame() {
     predictor->SaveStateForNextStartup();
 }
 
+void PredictorTabHelper::PreconnectUrl(const GURL& url) {
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  Predictor* predictor(profile->GetNetworkPredictor());
+  if (predictor && url.SchemeIsHTTPOrHTTPS())
+    predictor->PreconnectUrlAndSubresources(url, GURL());
+}
+
 }  // namespace chrome_browser_net

chrome/browser/net/predictor_tab_helper.h

@@ -6,9 +6,16 @@
 #define CHROME_BROWSER_NET_PREDICTOR_TAB_HELPER_H_
 
 #include "base/macros.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/reload_type.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
+namespace content {
+class NavigationHandle;
+}
+
 namespace chrome_browser_net {
 
 class PredictorTabHelper
@@ -18,12 +25,28 @@ class PredictorTabHelper
   ~PredictorTabHelper() override;
 
   // content::WebContentsObserver:
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override;
+  void DidStartNavigationToPendingEntry(
+      const GURL& url,
+      content::ReloadType reload_type) override;
   void DocumentOnLoadCompletedInMainFrame() override;
 
  private:
   explicit PredictorTabHelper(content::WebContents* web_contents);
   friend class content::WebContentsUserData<PredictorTabHelper>;
 
+  void PreconnectUrl(const GURL& url);
+
+  // This boolean is set to true after a call to
+  // DidStartNavigationToPendingEntry, which fires off predictive preconnects.
+  // This ensures that the resulting call to DidStartNavigation does not
+  // duplicate these preconnects. The tab helper spawns preconnects on the
+  // navigation to the pending entry because DidStartNavigation is called after
+  // render process spawn (pre-PlzNavigate), which can take substantial time
+  // especially on Android.
+  bool predicted_from_pending_entry_;
+
   DISALLOW_COPY_AND_ASSIGN(PredictorTabHelper);
 };