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..ea4b6e90dd8dc76c01e8324f915718e3aa195a75
--- /dev/null
+++ b/chrome/browser/renderer_host/predictor_resource_throttle.cc
@@ -0,0 +1,128 @@
+// 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 "base/memory/ptr_util.h"
+#include "chrome/browser/net/predictor.h"
+#include "chrome/browser/profiles/profile_io_data.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"
+
+namespace {
+
+bool IsNavigationRequest(net::URLRequest* request) {
+  content::ResourceType resource_type =
+      content::ResourceRequestInfo::ForRequest(request)->GetResourceType();
+  return resource_type == content::RESOURCE_TYPE_MAIN_FRAME ||
+         resource_type == content::RESOURCE_TYPE_SUB_FRAME;
+}
+
+}  // namespace
+
+PredictorResourceThrottle::PredictorResourceThrottle(
+    net::URLRequest* request,
+    chrome_browser_net::Predictor* predictor)
+    : request_(request), predictor_(predictor) {}
+
+PredictorResourceThrottle::~PredictorResourceThrottle() {}
+
+// static
+std::unique_ptr<PredictorResourceThrottle>
+PredictorResourceThrottle::MaybeCreate(net::URLRequest* request,
+                                       ProfileIOData* io_data) {
+  if (io_data->GetPredictor()) {
+    return base::WrapUnique(
+        new PredictorResourceThrottle(request, io_data->GetPredictor()));
+  }
+  return nullptr;
+}
+
+// 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);

[mmenke, 2016/05/24 20:27:34]

Not for this CL, but this is being called for non-navigation requests.  Perhaps
we should rename it.

[mmenke, 2016/05/24 20:27:34]

Also, worth noting there's a change in behavior here:  This method was being
called for all requests that go through the main URLRequestContext before.  Now
it's being called for all methods that go through the RDH.

So what requests are those?  Internal Chrome requests seem like the big one. 
Other requests are SDCH dictionary fetches (Which do have referrers, so could
also affect per-navigation learning).  There may be others (OCP requests?).

[mmenke, 2016/05/24 21:39:31]

Worth noting that SDCH dictionary requests do not set referrer, anyways, and
even if it did, it may be the referring resource, rather than the top level
resource, so we may miss them, anyways....unless the top level resource is the
one that refers to the SDCH dictionary.

[Charlie Harrison, 2016/05/24 21:43:22]

Added a TODO.

[Charlie Harrison, 2016/05/24 21:43:22]

Okay thanks for the explanation. It seems like this is not a terrible change in
behavior.

In any case, there's a tradeoff for preresolving user facing urls vs internal
urls. It's possible this change could be better for user experience, even (more
user facing results in the top ten startup list).

[mmenke, 2016/05/25 14:57:48]

Most internal requests won't have referrers, the question is just the
pseudo-internal requests, which may set referrer - I'm not sure there are any,
though.

[Charlie Harrison, 2016/05/25 15:01:08]

Gotcha. "LearnAboutInitialNavigation" doesn't care about referrers though.

[mmenke, 2016/05/25 15:21:20]

The rest of this code does, however...

If there are requests that set referrer and don't go through ResourceLoader,
they would have affected the predictor before, but after this CL, they won't.

There certainly are requests that don't set referrer and don't go through
ResourceLoader.  Those did result in calling LearnAboutInitialNavigation before,
and now they won't, so that is a change.

Hrm...Predictor's comments indicate initial_observer_ is only intended for
startup work, but I don't see it being deleted anywhere.  Is that a bug, or am I
missing something?

[mmenke, 2016/05/25 15:24:41]

And Charles pointed out to me that this is not an issue - initial_observer_ only
checks the first x "navigations" on startup.

+
+  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.

[mmenke, 2016/05/24 20:27:34]

This comment assumes this is a navigation request, but we haven't made that
check yet.  I suggest combining this check with the one below (And doing it
second, just because it makes logical sense, and it's marginally more
expensive), and merging this comment with the one below.

[Charlie Harrison, 2016/05/24 21:43:22]

Done.

+  if (referring_scheme_host == request_scheme_host)
+    return;
+
+  // Subresources for main frames are predicted when navigation starts, in
+  // PredictorTabHelper, so only handle predictions for subframes here.
+  // TODO(csharrison): When the PreconnectMore experiment is over, move all
+  // prediction dispatching to the tab helper.
+  if (IsNavigationRequest(request_)) {
+    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());
+    }
+  }
+}
+
+// Hooks into redirect notifications to learn and initiates predictions based on
+// 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())

[mmenke, 2016/05/24 20:27:34]

Hrm...I kinda wonder if it's possible for "new_scheme_host" to be empty here. 
Oh well, not relevant for this CL.

[Charlie Harrison, 2016/05/24 21:43:22]

Acknowledged.

+    return;
+  // Don't learn or predict subresource redirects.
+  if (!IsNavigationRequest(request_))
+    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);
+}
+
+const char* PredictorResourceThrottle::GetNameForLogging() const {
+  return "PredictorResourceThrottle";
+}