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 "content/public/browser/resource_request_info.h"
+#include "net/url_request/redirect_info.h"
+
+PredictorResourceThrottle::PredictorResourceThrottle(
+    net::URLRequest* request,
+    chrome_browser_net::Predictor* predictor)
+    : request_(request), predictor_(predictor) {}
+
+PredictorResourceThrottle::~PredictorResourceThrottle() {}
+
+void PredictorResourceThrottle::WillStartRequest(bool* defer) {

[mmenke, 2016/04/07 22:12:49]

Know this is just the old code, with the redirect stuff removed, but think you
could beef up the comments a bit?  It seems like a lot of code with little
explanation.

[Charlie Harrison, 2016/05/23 14:16:20]

Yeah sure no problem.

+  GURL request_scheme_host(

[mmenke, 2016/04/07 22:12:49]

include gurl.h

[Charlie Harrison, 2016/05/23 14:16:20]

Done.

+      chrome_browser_net::Predictor::CanonicalizeUrl(request_->url()));
+  if (request_scheme_host.is_empty())
+    return;
+  //

[mmenke, 2016/04/07 22:12:49]

nit:  Remove empty comment line - may make sense to have a blank line there.

[Charlie Harrison, 2016/05/23 14:16:20]

Done.

+  // 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();

[mmenke, 2016/04/07 22:12:49]

include content/public/common/resource_type.h

[Charlie Harrison, 2016/05/23 14:16:20]

Done.

+  bool is_main_frame = (resource_type == content::RESOURCE_TYPE_MAIN_FRAME);

[mmenke, 2016/04/07 22:12:49]

nit:  Can just get rid of this bool, and inline the check below.

[Charlie Harrison, 2016/05/23 14:16:20]

Done.

+
+  if (!request_->referrer().empty()) {
+    GURL referring_scheme_host = GURL(request_->referrer()).GetWithEmptyPath();
+    // Learn about our referring URL, for use in the future.
+    if (!is_main_frame &&
+        predictor_->timed_cache()->WasRecentlySeen(referring_scheme_host)) {
+      predictor_->LearnFromNavigation(referring_scheme_host,
+                                      request_scheme_host);
+    }
+    if (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 any preconnections
+      // need to be made at the first navigation (i.e., when referer was loaded)
+      // and wouldn't have waited for this current request navigation.
+      return;
+    }
+  }
+
+  // Subresources for main frames usually get predicted when we detected the
+  // main frame request - way back in RenderViewHost::Navigate.  So only handle
+  // predictions now for subframes.
+  if (resource_type == content::RESOURCE_TYPE_SUB_FRAME) {
+    predictor_->PredictFrameSubresources(request_scheme_host,
+                                         request_->first_party_for_cookies());
+  }
+  predictor_->timed_cache()->SetRecentlySeen(request_scheme_host);
+}
+
+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);

[mmenke, 2016/04/07 22:12:49]

Note that all of this code changes behavior, since we aren't currently doing any
of this on redirects.  Not sure we should include this stuff in this CL, without
tests.  Do we have integration tests for any of the old code (It's clear from
this CL we didn't have any unit tests)?

[Charlie Harrison, 2016/04/07 22:20:41]

Can you explain why the previous behavior does not track redirects?
OnBeforeURLRequest is called for redirects. As far as I can tell it is keyed off
URLRequest::Start.

[mmenke, 2016/04/07 22:39:59]

Looks like you're right.  Notice the "Only notify the delegate for the initial
request." comment in URLRequest::Start.  This was true for all of 3 weeks, at
which point it was made untrue by the same person who added the comment.  So now
we've had that incorrect comment hanging around for 5 years.  :(

Also, ResourceHandler::OnWillStart is only called once per request, and since
URLRequests are only started once from an external caller, I just assumed the
methods were analogous.  I had assumed redirects when through StartInternal,
instead of Start (Which they used to do - not sure if I remembered the
StartInternal call from 5 years ago or not, I may well have, since I was
learning the code at that time).

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