Refactor net predictor to use ResourceThrottles

chrome/browser/renderer_host/predictor_resource_throttle.cc

Index: chrome/browser/renderer_host/predictor_resource_throttle.cc
diff --git a/chrome/browser/renderer_host/predictor_resource_throttle.cc b/chrome/browser/renderer_host/predictor_resource_throttle.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a4c6f929d92c79e357ba21d9af2ceb06d97b102a
--- /dev/null
+++ b/chrome/browser/renderer_host/predictor_resource_throttle.cc
@@ -0,0 +1,98 @@
+// Copyright 2016 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/renderer_host/predictor_resource_throttle.h"
+
+#include "chrome/browser/net/predictor.h"
+#include "content/public/browser/resource_request_info.h"
+#include "content/public/common/resource_type.h"
+#include "net/url_request/redirect_info.h"
+#include "net/url_request/url_request.h"
+#include "url/gurl.h"
+
+PredictorResourceThrottle::PredictorResourceThrottle(
+    net::URLRequest* request,
+    chrome_browser_net::Predictor* predictor)
+    : request_(request), predictor_(predictor) {}
+
+PredictorResourceThrottle::~PredictorResourceThrottle() {}
+
+// Hooks into URLRequests starting to initiate predictions based on subframe
+// requests, and to learn referrer relationships.
+void PredictorResourceThrottle::WillStartRequest(bool* defer) {
+  GURL request_scheme_host(
+      chrome_browser_net::Predictor::CanonicalizeUrl(request_->url()));
+  if (request_scheme_host.is_empty())
+    return;
+
+  // Learn what URLs are likely to be needed during next startup.
+  predictor_->LearnAboutInitialNavigation(request_scheme_host);
+
+  const content::ResourceRequestInfo* info =
+      content::ResourceRequestInfo::ForRequest(request_);
+  DCHECK(info);
+  content::ResourceType resource_type = info->GetResourceType();
+  const GURL& referring_scheme_host =
+      GURL(request_->referrer()).GetWithEmptyPath();
+
+  // Learn about our referring URL, for use in the future. Only learn
+  // subresource relationships.
+  if (!referring_scheme_host.is_empty() &&
+      resource_type != content::RESOURCE_TYPE_MAIN_FRAME &&
+      predictor_->timed_cache()->WasRecentlySeen(referring_scheme_host)) {
+    predictor_->LearnFromNavigation(referring_scheme_host, request_scheme_host);
+  }
+
+  // We've already made any/all predictions when we navigated to the referring
+  // host, so we can bail out here. We don't update the RecentlySeen() time
+  // because the timed cache is already populated (with the correct timeout)
+  // based on the initial navigation.
+  if (referring_scheme_host == request_scheme_host)
+    return;
+
+  predictor_->timed_cache()->SetRecentlySeen(request_scheme_host);
+
+  // Subresources for main frames usually get predicted when we detected the

[mmenke, 2016/05/23 18:50:22]

Verb tenses are awkward here ("detected" is past tense, "get predicted" is not).
 Also, avoid "we".  Also note you mispelled predictions in the last line of this
comment."

Maybe "Subresources for main frames are predicted when navigation starts, in
PredictorTabHelper. So only handle predictions for subframes here".  Is there a
reason for not handling subframes there?  Or is it just that main frames have to
be handled there, and this is the "default" spot to handle that logic?  web
contents_observer does have DidRedirectNavigation, so could probably put all
making of predictions here, and all learning them here, so have all prediction
making in one spot.

[Charlie Harrison, 2016/05/23 21:13:32]

I think the tab helper was originally designed as an optimization, with all
other logic residing here. Your suggestion makes sense, but I'd like to make
that move when the PreconnectMore experiment is finished. That way, we can
inject the subframe logic in DidStartNavigation without messing with the
experiment.

+  // main frame request - way back in RenderViewHost::Navigate.  So only handle
+  // predsctions now for subframes.
+  predictor_->timed_cache()->SetRecentlySeen(request_scheme_host);
+  if (resource_type == content::RESOURCE_TYPE_SUB_FRAME) {
+    predictor_->PredictFrameSubresources(request_scheme_host,
+                                         request_->first_party_for_cookies());
+  }
+}
+
+// Hook into redirect notifications to learn and initiate predictions based on

[mmenke, 2016/05/23 18:50:22]

"Hooks", "initiates", like the comment for the method above.

[Charlie Harrison, 2016/05/23 21:13:32]

Done.

+// the redirected url.
+void PredictorResourceThrottle::WillRedirectRequest(
+    const net::RedirectInfo& redirect_info,
+    bool* defer) {
+  GURL new_scheme_host(
+      chrome_browser_net::Predictor::CanonicalizeUrl(redirect_info.new_url));
+  GURL original_scheme_host(request_->original_url().GetWithEmptyPath());
+  if (new_scheme_host == original_scheme_host || new_scheme_host.is_empty())
+    return;
+  // Don't learn from redirects that take path as an argument, but do
+  // learn from short-hand typing entries, such as "cnn.com" redirects to
+  // "www.cnn.com".  We can't just check for has_path(), as a mere "/"
+  // will count as a path, so we check that the path is at most a "/"
+  // (1 character long) to decide the redirect is "definitive" and has no
+  // significant path.
+  // TODO(jar): It may be ok to learn from all redirects, as the adaptive
+  // system will not respond until several identical redirects have taken
+  // place.  Hence a use of a path (that changes) wouldn't really be
+  // learned from anyway.
+  if (request_->original_url().path().length() <= 1 &&
+      predictor_->timed_cache()->WasRecentlySeen(original_scheme_host)) {
+    // TODO(jar): These definite redirects could be learned much faster.
+    predictor_->LearnFromNavigation(original_scheme_host, new_scheme_host);
+  }
+  predictor_->timed_cache()->SetRecentlySeen(new_scheme_host);
+  predictor_->PredictFrameSubresources(
+      new_scheme_host, redirect_info.new_first_party_for_cookies);

[mmenke, 2016/05/23 18:50:22]

I don't think this should be called on non-main-frame / non-sub-frame resources.
 I don't think our tests are currently capable of catching that bug.  :(

[Charlie Harrison, 2016/05/23 21:13:32]

Wow you're right, even though this is the current behavior of the predictor.
Added a test that catches this.

+}
+
+const char* PredictorResourceThrottle::GetNameForLogging() const {
+  return "PredictorResourceThrottle";
+}