Change Interceptors into URLRequestJobFactory::ProtocolHandlers.

chrome/browser/net/connect_interceptor.cc

 // Copyright (c) 2011 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/net/connect_interceptor.h"
 
 #include "chrome/browser/net/predictor.h"
 #include "net/base/load_flags.h"
+#include "net/url_request/protocol_intercept_job_factory.h"
 #include "net/url_request/url_request.h"
 
 namespace chrome_browser_net {
 
 // We don't bother learning to preconnect via a GET if the original URL
 // navigation was so long ago, that a preconnection would have been dropped
 // anyway.  We believe most servers will drop the connection in 10 seconds, so
 // we currently estimate this time-till-drop at 10 seconds.
 // TODO(jar): We should do a persistent field trial to validate/optimize this.
 static const int kMaxUnusedSocketLifetimeSecondsWithoutAGet = 10;
 
+// static
+scoped_ptr<net::URLRequestJobFactory>
+ConnectInterceptor::CreateURLRequestJobFactory(
+    scoped_ptr<net::URLRequestJobFactory> base_job_factory,
+    Predictor* predictor) {
+  scoped_ptr<net::URLRequestJobFactory> top_job_factory(
+      new net::ProtocolInterceptJobFactory(base_job_factory.Pass(),
+          new ConnectInterceptor(predictor)));
+  return top_job_factory.Pass();
+}
+
 ConnectInterceptor::ConnectInterceptor(Predictor* predictor)
     : timed_cache_(base::TimeDelta::FromSeconds(
           kMaxUnusedSocketLifetimeSecondsWithoutAGet)),
       predictor_(predictor) {
   DCHECK(predictor);
 }
 
 ConnectInterceptor::~ConnectInterceptor() {
 }
 
-net::URLRequestJob* ConnectInterceptor::MaybeIntercept(
+net::URLRequestJob* ConnectInterceptor::MaybeCreateJob(

[willchan no longer on Chromium, 2012/12/11 19:14:38]

No, this ConnectInterceptor is all wrong :) When cleaning it up we should Do It
Right (TM) :P

If you look at this function, you'll see it *always* returns NULL. The purpose
of this interceptor is to *observe*, not to create jobs. In other words, it's
abusing the interceptor interface. Rather than reworking it into a
URLRequestJobFactory, we should use the ChromeNetworkDelegate instead.

[pauljensen, 2012/12/11 22:28:14]

Right now the ConnectionInterceptor is installed into the
URLRequestJobFactoryImpl for the extension_context (and most other contexts) but
the ChromeNetworkDelegate is not installed in the extension_context.
The ConnectionInterceptor is part of the whole predictor framework so I assume
it's most important that it watches URLs going through the main_context but I
don't know if this difference with the extension_context matters.  The comment
by GetRequestContextForExtensions() says "This is only used for a separate
cookie store currently." making me think this isn't a big deal.  Do you guys
think it should be okay to move ConnectionInterceptor into ChromeNetworkDelegate
and not worry about this difference?  There are other alternatives.

[mmenke, 2012/12/13 16:12:04]

I think that's fine for now, though should be done in another CL  (Which is what
I assume you were thinking anyways, as you removed the files from this one).

[pauljensen, 2012/12/13 17:53:30]

Sorry, I'm confused.
What is fine for now? Moving ConnectionInterceptor to the ChromeNetworkDelegate
in another CL and not worrying about the fact that it's not connected to the
extension_context?
What files did I remove from this one?

[willchan no longer on Chromium, 2012/12/13 17:59:53]

So, long-term we need to start refactoring ChromeNetworkDelegate. We've kinda
overloaded it now. It's shared by the "system" URLRequestContext, the profiles'
main & app URLRequestContexts, but not the extension one. What we want to do is
probably use composition here to share behavior across different NetworkDelegate
implementations.

But I didn't answer your question yet. AIUI, the extension context is purely for
the chrome-extension:// to override the CookieStore. So it's not a big deal not
to let ConnectionInterceptor hook it. Long-term, I imagine the CookieStore being
part of the dependencies passed directly into a ProtocolHandler, so there won't
be a need to create a whole new URLRequestContext just for extensions.

Make sense? If people agree, can y'all file bugs? Don't need to sign up to
tackle it yourselves unless you'd like to. I have some other folks who may be
interested, but you're doing such a great job here I'd rather you continue to
kick ass and take names :)

I defer to the main reviewers about whether or not such work would make sense to
be done in this CL or another.

[pauljensen, 2012/12/13 18:21:54]

If I move ConnectionInterceptor into the ChromeNetworkDelegate in this CL I can
yank out all my changes to pass around the URLRequestJobFactories through
SetupJobFactory() and SetupJobFactoryDefaults().  Just sayin' :)  I'll also
defer to the main reviewers to make the call.

     net::URLRequest* request, net::NetworkDelegate* network_delegate) const {
   GURL request_scheme_host(Predictor::CanonicalizeUrl(request->url()));
   if (request_scheme_host == GURL::EmptyGURL())
     return NULL;
 
   // Learn what URLs are likely to be needed during next startup.
   predictor_->LearnAboutInitialNavigation(request_scheme_host);
 
   bool redirected_host = false;
   if (request->referrer().empty()) {
@@ skipping 39 matching lines @@
   timed_cache_.SetRecentlySeen(request_scheme_host);
 
   // 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 subresources or for redirected hosts.
   if ((request->load_flags() & net::LOAD_SUB_FRAME) || redirected_host)
     predictor_->PredictFrameSubresources(request_scheme_host);
   return NULL;
 }
 
-net::URLRequestJob* ConnectInterceptor::MaybeInterceptResponse(
-    net::URLRequest* request, net::NetworkDelegate* network_delegate) const {
-  return NULL;
-}
-
-net::URLRequestJob* ConnectInterceptor::MaybeInterceptRedirect(
-    const GURL& location,
-    net::URLRequest* request,
-    net::NetworkDelegate* network_delegate) const {
-  return NULL;
-}
-
 ConnectInterceptor::TimedCache::TimedCache(const base::TimeDelta& max_duration)
     : mru_cache_(UrlMruTimedCache::NO_AUTO_EVICT),
       max_duration_(max_duration) {
 }
 
 // Make Clang compilation happy with explicit destructor.
 ConnectInterceptor::TimedCache::~TimedCache() {}
 
 bool ConnectInterceptor::TimedCache::WasRecentlySeen(const GURL& url) const {
   DCHECK_EQ(url.GetWithEmptyPath(), url);
   // Evict any overly old entries.
   base::TimeTicks now = base::TimeTicks::Now();
   UrlMruTimedCache::reverse_iterator eldest = mru_cache_.rbegin();
   while (!mru_cache_.empty()) {
     DCHECK(eldest == mru_cache_.rbegin());
     if (now - eldest->second < max_duration_)
       break;
     eldest = mru_cache_.Erase(eldest);
   }
   return mru_cache_.end() != mru_cache_.Peek(url);
 }
 
 void ConnectInterceptor::TimedCache::SetRecentlySeen(const GURL& url) const {
   DCHECK_EQ(url.GetWithEmptyPath(), url);
   mru_cache_.Put(url, base::TimeTicks::Now());
 }
 
 }  // namespace chrome_browser_net