Refactor net predictor to use ResourceThrottles

chrome/browser/renderer_host/predictor_resource_throttle.cc

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

[mmenke, 2016/05/25 18:16:04]

I don't think either of these method-level comments adds any value - they're
basically covered by the class description.

[Charlie Harrison, 2016/05/25 18:45:15]

Done.

+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.
+  // TODO(csharrison): Rename this method, as this code path is used for more
+  // than just navigation requests.
+  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);
+  }
+
+  // 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.

[mmenke, 2016/05/25 18:16:04]

Having a TODO in the middle of two informational comments can be confusing. 
Maybe move this to the header, below the class description?  Or just below both
other comments here.

[Charlie Harrison, 2016/05/25 18:45:16]

Done.

+  // 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
+  // populated (with the correct timeout) based on the initial navigation.
+  if (IsNavigationRequest(request_) &&
+      referring_scheme_host != request_scheme_host) {
+    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/25 18:16:04]

BUG:  We're comparing a non-canonicalized URL with a canonicalized one.  I'd
suggest keeping it as-is for now, but think the whole canonicalization thing
here requires more thought.

[Charlie Harrison, 2016/05/25 18:45:16]

Added a note.

+    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";
+}