Change Interceptors into URLRequestJobFactory::ProtocolHandlers.

Convert trivial URLRequest::Interceptors and URLRequestJobFactory::Interceptor into URLRequestJobFactory::ProtocolHandlers.

BUG=161524
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174174

[pauljensen, 2012-11-13 17:21:50]

Erik, before I go and start converting all the simple Interceptors, can you take
a high-level look at this first conversion?

[pauljensen, 2012-11-13 17:27:07]

I just realized this isn't thread safe. 
URLRequestJobManager::RegisterRequestInterceptor() (and in my code
URLRequestFilter::AddHostnameProtocolHandler() ) has locking as its called from
the UI thread.  I'll need to address this.

[pauljensen, 2012-11-13 19:12:18]

I think I addressed the threading issues.
Very sadly the original code was not thread
safe...URLRequestJobManager::IsAllowedThread() is disabled and has been since
2008 (http://b/issue?id=1338969) and there is only locking on half of the uses
of URLRequestJobManager::interceptors_.

[pauljensen, 2012-11-13 22:11:58]

To accommodate rewriting other Interceptors I'm starting to think
URLRequestFilter should just have a list of ProtocolHandlers rather than mapping
them only for certain URLs or hosts.

[erikwright (departed), 2012-11-14 19:33:23]

On 2012/11/13 22:11:58, pauljensen wrote:
> To accommodate rewriting other Interceptors I'm starting to think
> URLRequestFilter should just have a list of ProtocolHandlers rather than
mapping
> them only for certain URLs or hosts.

That seems fine to me. And they would just create a job or not, depending on
whether the url matches.

Presumably we would provide some utility classes to make it easy to intercept
specific URLs.

Also presumably, we can support the current API by creating the URL-specific
ProtocolHandler when we receive a legacy factory function.

It's also important that we provide a way to turn on the URLRequestFilter
without going through the static registration methods on URLRequestJobManager.
(i.e., putting a placeholder JobFactory on all contexts that is a no-op unless
some interceptors are registered on URLRequestFilter).

[pauljensen, 2012-11-20 17:17:36]

erik, PTAL.

[erikwright (departed), 2012-11-21 22:03:36]

Sorry, Paul, was swamped today and yesterday.

Will get to this before you are back on Monday. Happy Thanksgiving!

[pauljensen, 2012-11-26 13:56:38]

On 2012/11/21 22:03:36, erikwright wrote:
> Sorry, Paul, was swamped today and yesterday.
> 
> Will get to this before you are back on Monday. Happy Thanksgiving!

I'm back :)

Two thoughts that popped into my head after some turkey:
1. ProtocolInterceptJobFactory::IsHandledProtocol and
ProtocolInterceptJobFactory::IsHandledURL should probably just call the
job_factory_ equivalents rather than trying to handle scheme_.  This better
matches how the replaced Interceptors worked.
2. URLRequestFilter should probably own the ProtocolHandlers it manages, and
should delete them when they are unregistered.  This might avoid the ASAN
failure (which I'll look into more today).

[erikwright (departed), 2012-11-27 05:54:05]

First comments. I haven't looked at all the individual conversions yet.

https://codereview.chromium.org/11293252/diff/15072/net/url_request/protocol_...
File net/url_request/protocol_intercept_job_factory.cc (right):

https://codereview.chromium.org/11293252/diff/15072/net/url_request/protocol_...
net/url_request/protocol_intercept_job_factory.cc:16: const char* kHttpScheme =
"http";
These are no longer used here, right?

https://codereview.chromium.org/11293252/diff/15072/net/url_request/protocol_...
net/url_request/protocol_intercept_job_factory.cc:32: return
job_factory_->SetProtocolHandler(scheme, protocol_handler);
I'm not thrilled about this, although I understand why.

See this other CL where these methods are removed:

https://codereview.chromium.org/11227017/

I've added you to the thread where we discuss the other change that is blocking
that CL.

https://codereview.chromium.org/11293252/diff/15072/net/url_request/url_reque...
File net/url_request/url_request_filter.h (right):

https://codereview.chromium.org/11293252/diff/15072/net/url_request/url_reque...
net/url_request/url_request_filter.h:39:
base::Callback<URLRequest::ProtocolFactory> > HostnameHandlerMap;
include callback.h

https://codereview.chromium.org/11293252/diff/15072/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11293252/diff/15072/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:473: job_factory_.reset(new
URLRequestJobFactoryImpl);
I assume this wrapped factory exists because someone later adds protocol
handlers?

And that this would be a blocker for removing the mutation methods?

Where are they added and can we avoid that? (i.e., by getting a populated
jobfactory before creating the interceptjobfactory?)

[pauljensen, 2012-11-30 21:02:34]

Erik, PTAL.

https://codereview.chromium.org/11293252/diff/15072/net/url_request/protocol_...
File net/url_request/protocol_intercept_job_factory.cc (right):

https://codereview.chromium.org/11293252/diff/15072/net/url_request/protocol_...
net/url_request/protocol_intercept_job_factory.cc:16: const char* kHttpScheme =
"http";
On 2012/11/27 05:54:05, erikwright wrote:
> These are no longer used here, right?

Done.

https://codereview.chromium.org/11293252/diff/15072/net/url_request/url_reque...
File net/url_request/url_request_filter.h (right):

https://codereview.chromium.org/11293252/diff/15072/net/url_request/url_reque...
net/url_request/url_request_filter.h:39:
base::Callback<URLRequest::ProtocolFactory> > HostnameHandlerMap;
On 2012/11/27 05:54:05, erikwright wrote:
> include callback.h

Done.

https://codereview.chromium.org/11293252/diff/15072/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11293252/diff/15072/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:473: job_factory_.reset(new
URLRequestJobFactoryImpl);
On 2012/11/27 05:54:05, erikwright wrote:
> I assume this wrapped factory exists because someone later adds protocol
> handlers?
> 
> And that this would be a blocker for removing the mutation methods?
> 
> Where are they added and can we avoid that? (i.e., by getting a populated
> jobfactory before creating the interceptjobfactory?)

Nobody adds protocol handlers later and job_factory_ is a URLRequestJobFactory
(i.e. not a URLRequestJobFactoryImpl) so this shouldn't hold up
http://crrev.com/11227017

[pauljensen, 2012-12-06 14:18:07]

Erik, ping.

[erikwright (departed), 2012-12-06 15:26:28]

All looking pretty good. Still confused about one bit.

We can chat by IM or VC a bit later if necessary.

https://codereview.chromium.org/11293252/diff/22109/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/22109/chrome/browser/component_...
chrome/browser/component_updater/component_updater_interceptor.cc:19: Delegate()
: hit_count_(0) {}
It might be appropriate to DCHECK that there is only ever one registered
instance of Delegate at a time.

https://codereview.chromium.org/11293252/diff/22109/chrome/browser/extensions...
File chrome/browser/extensions/autoupdate_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/22109/chrome/browser/extensions...
chrome/browser/extensions/autoupdate_interceptor.cc:12: #include
"net/url_request/url_request_filter.h"
It might be worth it to take this opportunity to de-duplicate this class and the
component_updater one.

I have a feeling you'll be back here later, so by de-duplicating now you might
save some work.

https://codereview.chromium.org/11293252/diff/22109/content/test/net/url_requ...
File content/test/net/url_request_abort_on_end_job.cc (right):

https://codereview.chromium.org/11293252/diff/22109/content/test/net/url_requ...
content/test/net/url_request_abort_on_end_job.cc:35: void
URLRequestAbortOnEndJob::AddUrlHandlerOnIOThread() {
This could be a free function in anonymous namespace to avoid polluting the
class declaration.

Unless it also makes sense to make Factory a private function, in which case
this would need to stay a member.

Ultimately, I would make this and Factory free functions in anon namespace and
make the constructor public, assuming that with this change there are no
remaining direct uses of the Factory.

https://codereview.chromium.org/11293252/diff/22109/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11293252/diff/22109/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:473: job_factory_.reset(new
URLRequestJobFactoryImpl);
Is there a reason why this should be a ProtocolInterceptJobFactory wrapping an
empty URLRequestJobFactoryImpl if no-one ever accesses the mutators?

Couldn't it just be an URLRequestJobFactoryImpl with a single ProtocolHandler
then?

Or couldn't we do one of:

1) make the mutators in ProtocolInterceptJobFactory not forward to the wrapped
factory, or
2) create a new URLRequestJobFactory implementation that only holds a single
ProtocolHandler and NOTREACHED()'s all mutators?

[pauljensen, 2012-12-07 18:47:42]

Erik, PTAL.

https://codereview.chromium.org/11293252/diff/22109/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/22109/chrome/browser/component_...
chrome/browser/component_updater/component_updater_interceptor.cc:19: Delegate()
: hit_count_(0) {}
On 2012/12/06 15:26:28, erikwright wrote:
> It might be appropriate to DCHECK that there is only ever one registered
> instance of Delegate at a time.
Done.  I did this by having URLRequestFilter DCHECK if multiple handlers are
added for a single URL/hostname.

https://codereview.chromium.org/11293252/diff/22109/chrome/browser/extensions...
File chrome/browser/extensions/autoupdate_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/22109/chrome/browser/extensions...
chrome/browser/extensions/autoupdate_interceptor.cc:12: #include
"net/url_request/url_request_filter.h"
On 2012/12/06 15:26:28, erikwright wrote:
> It might be worth it to take this opportunity to de-duplicate this class and
the
> component_updater one.
> 
> I have a feeling you'll be back here later, so by de-duplicating now you might
> save some work.

Done.

https://codereview.chromium.org/11293252/diff/22109/content/test/net/url_requ...
File content/test/net/url_request_abort_on_end_job.cc (right):

https://codereview.chromium.org/11293252/diff/22109/content/test/net/url_requ...
content/test/net/url_request_abort_on_end_job.cc:35: void
URLRequestAbortOnEndJob::AddUrlHandlerOnIOThread() {
On 2012/12/06 15:26:28, erikwright wrote:
> This could be a free function in anonymous namespace to avoid polluting the
> class declaration.
> 
> Unless it also makes sense to make Factory a private function, in which case
> this would need to stay a member.
> 
> Ultimately, I would make this and Factory free functions in anon namespace and
> make the constructor public, assuming that with this change there are no
> remaining direct uses of the Factory.

Done.

https://codereview.chromium.org/11293252/diff/22109/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11293252/diff/22109/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:473: job_factory_.reset(new
URLRequestJobFactoryImpl);
On 2012/12/06 15:26:28, erikwright wrote:
> Is there a reason why this should be a ProtocolInterceptJobFactory wrapping an
> empty URLRequestJobFactoryImpl if no-one ever accesses the mutators?
> 
> Couldn't it just be an URLRequestJobFactoryImpl with a single ProtocolHandler
> then?
TestJobInterceptor only intercepts the first HTTP request, any subsequent HTTP
requests or non-HTTP requests will be handled by the default static
URLRequest::ProtocolFactory's in URLRequestJobManager.  I was trying to maintain
this exact behavior.  After looking at users of TestJobInterceptor, it looks
like the tests don't actually need to rely on the default static
URLRequest::ProtocolFactory's, so I guess the answer is I could just call
URLRequestJobFactoryImpl::SetProtocolHandler().
> 
> Or couldn't we do one of:
> 
> 1) make the mutators in ProtocolInterceptJobFactory not forward to the wrapped
> factory, or
> 2) create a new URLRequestJobFactory implementation that only holds a single
> ProtocolHandler and NOTREACHED()'s all mutators?
Why the concern about mutators?  I thought these were going away imminently.

[erikwright (departed), 2012-12-07 20:12:05]

LG.

Clearly you will be needing some other reviewers. willchan will care about the
high-level though I suspect he will leave the details to Matt. I'd suggest going
through them before bothering Android, Content, etc. OWNERS.

https://codereview.chromium.org/11293252/diff/22109/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11293252/diff/22109/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:473: job_factory_.reset(new
URLRequestJobFactoryImpl);
On 2012/12/07 18:47:42, pauljensen wrote:
> On 2012/12/06 15:26:28, erikwright wrote:
> > Is there a reason why this should be a ProtocolInterceptJobFactory wrapping
an
> > empty URLRequestJobFactoryImpl if no-one ever accesses the mutators?
> > 
> > Couldn't it just be an URLRequestJobFactoryImpl with a single
ProtocolHandler
> > then?
> TestJobInterceptor only intercepts the first HTTP request, any subsequent HTTP
> requests or non-HTTP requests will be handled by the default static
> URLRequest::ProtocolFactory's in URLRequestJobManager.  I was trying to
maintain
> this exact behavior.  After looking at users of TestJobInterceptor, it looks
> like the tests don't actually need to rely on the default static
> URLRequest::ProtocolFactory's, so I guess the answer is I could just call
> URLRequestJobFactoryImpl::SetProtocolHandler().

OK, I see.

> > Or couldn't we do one of:
> > 
> > 1) make the mutators in ProtocolInterceptJobFactory not forward to the
wrapped
> > factory, or
> > 2) create a new URLRequestJobFactory implementation that only holds a single
> > ProtocolHandler and NOTREACHED()'s all mutators?
> Why the concern about mutators?  I thought these were going away imminently.

It's just that I noticed that you had converted the mutators in the
ProtocolInterceptJobFactory from NOTREACHED to forwarding, which leads me to
wonder who's relying on them (as they will presumably present a problem when we
do attempt to remove them).

If it doesn't break anyone, I'd leave the NOTREACHED, but ultimately I'm also OK
to worry about that separately.

https://codereview.chromium.org/11293252/diff/44001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/11293252/diff/44001/content/content_tests.gyp...
content/content_tests.gypi:119:
'test/net/url_request_prepackaged_interceptor.cc',
I wonder if this belongs in content/ or in chrome/test/base since it's only used
by chrome/ tests. But I'm happy to not move it for now and ask the opinion of
other reviewers.

https://codereview.chromium.org/11293252/diff/44001/content/test/net/url_requ...
File content/test/net/url_request_prepackaged_interceptor.h (right):

https://codereview.chromium.org/11293252/diff/44001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.h:16: // This url request
interceptor lets us respond to localhost http request urls
url -> URL, http -> HTTP, urls -> URLs

The preferred form would be something like:

"Intercepts HTTP requests and gives pre-defined responses to specified URLs."

Mention somewhere that the configured responses are active for the lifetime of
this class.

https://codereview.chromium.org/11293252/diff/44001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.h:28: void
SetQuerylessResponse(const std::string& url,
Add comment. Consider Renaming to SetResponseIgnoreQuery

https://codereview.chromium.org/11293252/diff/44001/net/url_request/protocol_...
File net/url_request/protocol_intercept_job_factory.h (right):

https://codereview.chromium.org/11293252/diff/44001/net/url_request/protocol_...
net/url_request/protocol_intercept_job_factory.h:55: const char* scheme_;
I'm not sure what the convention for this kind of thing is, but it seems risky
to accept and store a const char*. If someone calls you like this, it's trouble:

std::string scheme = ...;
... new ProtocolInterceptJobFactory(..., scheme.c_str(), ...);

Probably best to store a std::string, and possibly to accept one (or accept a
StringPiece, which can be implicitly constructed from a string or a const
char*).

[pauljensen, 2012-12-09 23:14:12]

I addressed Erik's comments.  Matt and Will, PTAL.  Will, if you just want to do
a high-level review that's fine.

https://codereview.chromium.org/11293252/diff/22109/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11293252/diff/22109/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:473: job_factory_.reset(new
URLRequestJobFactoryImpl);
On 2012/12/07 20:12:05, erikwright wrote:
> On 2012/12/07 18:47:42, pauljensen wrote:
> > On 2012/12/06 15:26:28, erikwright wrote:
> > > Is there a reason why this should be a ProtocolInterceptJobFactory
wrapping
> an
> > > empty URLRequestJobFactoryImpl if no-one ever accesses the mutators?
> > > 
> > > Couldn't it just be an URLRequestJobFactoryImpl with a single
> ProtocolHandler
> > > then?
> > TestJobInterceptor only intercepts the first HTTP request, any subsequent
HTTP
> > requests or non-HTTP requests will be handled by the default static
> > URLRequest::ProtocolFactory's in URLRequestJobManager.  I was trying to
> maintain
> > this exact behavior.  After looking at users of TestJobInterceptor, it looks
> > like the tests don't actually need to rely on the default static
> > URLRequest::ProtocolFactory's, so I guess the answer is I could just call
> > URLRequestJobFactoryImpl::SetProtocolHandler().
> 
> OK, I see.
> 
> > > Or couldn't we do one of:
> > > 
> > > 1) make the mutators in ProtocolInterceptJobFactory not forward to the
> wrapped
> > > factory, or
> > > 2) create a new URLRequestJobFactory implementation that only holds a
single
> > > ProtocolHandler and NOTREACHED()'s all mutators?
> > Why the concern about mutators?  I thought these were going away imminently.
> 
> It's just that I noticed that you had converted the mutators in the
> ProtocolInterceptJobFactory from NOTREACHED to forwarding, which leads me to
> wonder who's relying on them (as they will presumably present a problem when
we
> do attempt to remove them).
> 
> If it doesn't break anyone, I'd leave the NOTREACHED, but ultimately I'm also
OK
> to worry about that separately.
For now they must pass things like SetProtocolHandler() onto |job_factory_|
until crrev.com/11308362 lands.

https://codereview.chromium.org/11293252/diff/44001/content/test/net/url_requ...
File content/test/net/url_request_prepackaged_interceptor.h (right):

https://codereview.chromium.org/11293252/diff/44001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.h:16: // This url request
interceptor lets us respond to localhost http request urls
On 2012/12/07 20:12:05, erikwright wrote:
> url -> URL, http -> HTTP, urls -> URLs
> 
> The preferred form would be something like:
> 
> "Intercepts HTTP requests and gives pre-defined responses to specified URLs."
> 
> Mention somewhere that the configured responses are active for the lifetime of
> this class.
I didn't change this (it's from autoupdate_interceptor.h and
component_update_interceptor.h).  I've implemented your suggestions.

https://codereview.chromium.org/11293252/diff/44001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.h:28: void
SetQuerylessResponse(const std::string& url,
On 2012/12/07 20:12:05, erikwright wrote:
> Add comment. Consider Renaming to SetResponseIgnoreQuery

Done.

https://codereview.chromium.org/11293252/diff/44001/net/url_request/protocol_...
File net/url_request/protocol_intercept_job_factory.h (right):

https://codereview.chromium.org/11293252/diff/44001/net/url_request/protocol_...
net/url_request/protocol_intercept_job_factory.h:55: const char* scheme_;
On 2012/12/07 20:12:05, erikwright wrote:
> I'm not sure what the convention for this kind of thing is, but it seems risky
> to accept and store a const char*. If someone calls you like this, it's
trouble:
> 
> std::string scheme = ...;
> ... new ProtocolInterceptJobFactory(..., scheme.c_str(), ...);
> 
> Probably best to store a std::string, and possibly to accept one (or accept a
> StringPiece, which can be implicitly constructed from a string or a const
> char*).
|scheme_| isn't really necessary...I'll remove it all together.

[mmenke, 2012-12-11 17:22:38]

https://codereview.chromium.org/11293252/diff/46001/android_webview/native/an...
File android_webview/native/android_protocol_handler.cc (right):

https://codereview.chromium.org/11293252/diff/46001/android_webview/native/an...
android_webview/native/android_protocol_handler.cc:231:
scoped_ptr<net::URLRequestJobFactory> RegisterAndroidProtocolsOnIOThread(
Since we're creating a new factory, I think this function name is a bit of a
misnomer.

"CreateAndroidRequestJobFactoryOnIOThread"?  Or maybe just
"CreateAndroidRequestJobFactory"?

https://codereview.chromium.org/11293252/diff/46001/android_webview/native/an...
android_webview/native/android_protocol_handler.cc:232: net::URLRequestContext*
context,
While we're here, looks like we don't need |context|.

https://codereview.chromium.org/11293252/diff/46001/android_webview/native/co...
File android_webview/native/cookie_manager.h (right):

https://codereview.chromium.org/11293252/diff/46001/android_webview/native/co...
android_webview/native/cookie_manager.h:21: }  // namespace android_webview;
While you're here, please remove the semi-colon.

https://codereview.chromium.org/11293252/diff/46001/android_webview/native/ne...
File android_webview/native/net_init_native_callback.cc (right):

https://codereview.chromium.org/11293252/diff/46001/android_webview/native/ne...
android_webview/native/net_init_native_callback.cc:13:
scoped_ptr<net::URLRequestJobFactory> OnNetworkStackInitialized(
I think having an "OnBlah" function return a value is pretty confusing and
unexpected.  It's not at all clear from the name just what it's actually
returning.

Maybe CreateAndroidJobFactoryAndInitializeContext?  I'm open to better ideas.

https://codereview.chromium.org/11293252/diff/46001/android_webview/native/ne...
android_webview/native/net_init_native_callback.cc:21: }
nit:  While you're here, this should have a "// namespace android_webview"

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_interceptor.h (left):

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_interceptor.h:3: // found in
the LICENSE file.
Hmm...This file is labelled "M" rather than "D".

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
File content/test/net/url_request_prepackaged_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:36: bool ignore_query) {
BrowserThread::CurrentlyOn(BrowserThread::IO)?

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:62:
EXPECT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO));
May actually want to use CHECK here.  If we're on the wrong thread, things have
gone badly wrong in the test framework.

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:65: return NULL;
nit:  Fix indent.

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:70:
base::ThreadRestrictions::ScopedAllowIO allow_io;
Suggest moving this to just before ReadFileToString, as that seems to be the
only place it's needed.

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:92:
EXPECT_TRUE(file_util::ReadFileToString(response, &contents));
Can we just use a "URLRequestMockHTTPJob" here?

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:124: const FilePath&
path) {
Suggest a DCHECK_EQ on url.hostname

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
File content/test/net/url_request_prepackaged_interceptor.h (right):

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.h:33: const FilePath&
path);
nit:  Could use a single line here and above.

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.h:41: Delegate* delegate_;
Think it's worth mentioning that after creation, this lives on the IO thread,
and a task to delete it is posted on destruction of |this|.

https://codereview.chromium.org/11293252/diff/46001/net/proxy/proxy_script_fe...
File net/proxy/proxy_script_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/11293252/diff/46001/net/proxy/proxy_script_fe...
net/proxy/proxy_script_fetcher_impl_unittest.cc:84:
url_request_job_factory_.Pass(),
optional:  This certainly works, but I think storing the temporary in
url_request_job_factory_ makes the code a little harder to follow.

Why not just use
"scoped_ptr<URLRequestJobFactory>(new URLRequestJobFactoryImpl)" here?  If you
prefer your code, that's fine.

https://codereview.chromium.org/11293252/diff/46001/net/url_request/protocol_...
File net/url_request/protocol_intercept_job_factory.h (right):

https://codereview.chromium.org/11293252/diff/46001/net/url_request/protocol_...
net/url_request/protocol_intercept_job_factory.h:28: ProtocolHandler*
protocol_handler);
Since we're taking ownership of protocol_handler just like job_factory,
shouldn't they both be scoped_ptrs?

https://codereview.chromium.org/11293252/diff/46001/net/url_request/url_reque...
File net/url_request/url_request_filter_unittest.cc (right):

https://codereview.chromium.org/11293252/diff/46001/net/url_request/url_reque...
net/url_request/url_request_filter_unittest.cc:36: }
For the sake of completeness, suggest either turning one of these into a
ProtocolHandler, or adding a ProtocolHandler.

https://codereview.chromium.org/11293252/diff/46001/net/url_request/url_reque...
File net/url_request/url_request_test_util.h (right):

https://codereview.chromium.org/11293252/diff/46001/net/url_request/url_reque...
net/url_request/url_request_test_util.h:303: // A simple Interceptor that
returns a pre-built URLRequestJob only once.
Interceptor -> ProtocolHandler.

[willchan no longer on Chromium, 2012-12-11 19:14:37]

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/net/connec...
File chrome/browser/net/connect_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/net/connec...
chrome/browser/net/connect_interceptor.cc:42: net::URLRequestJob*
ConnectInterceptor::MaybeCreateJob(
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.

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:561: // that we can later
provide our own handles for storage related protocols.
s/handles/handlers/

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:568: top_job_factory =
SetUpJobFactory(base_job_factory.Pass(),
Now that you're returning it, perhas we should rename to CreateJobFactory()

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:675:
SetUpJobFactoryDefaults(job_factory.Pass(),
Ditto here. Maybe this should be CreateDefaultJobFactory()

[mmenke, 2012-12-11 19:18:25]

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_interceptor.h (left):

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_interceptor.h:3: // found in
the LICENSE file.
On 2012/12/11 17:22:38, Matt Menke wrote:
> Hmm...This file is labelled "M" rather than "D".

Clarification:  In the list of changed files, it's marked as modified rather
than deleted.

[willchan no longer on Chromium, 2012-12-11 19:23:34]

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/extensions...
File chrome/browser/extensions/autoupdate_interceptor.h (left):

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/extensions...
chrome/browser/extensions/autoupdate_interceptor.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
Why is this not marked [D]eleted?

https://codereview.chromium.org/11293252/diff/46001/webkit/tools/test_shell/t...
File webkit/tools/test_shell/test_shell.cc (right):

https://codereview.chromium.org/11293252/diff/46001/webkit/tools/test_shell/t...
webkit/tools/test_shell/test_shell.cc:172: net::URLRequestFilter* filter =
net::URLRequestFilter::GetInstance();
Whoa! What's this global here? I've never seen this in all my years on the team.
Let's kill this (I'm not saying you have to do this in this CL, just remarking
this so our team can keep it in mind).

[pauljensen, 2012-12-11 21:22:55]

I addressed mmenke's first round of comments.

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_interceptor.h (left):

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_interceptor.h:3: // found in
the LICENSE file.
On 2012/12/11 17:22:38, Matt Menke wrote:
> Hmm...This file is labelled "M" rather than "D".
Ya I've noticed this.  "git cl patch 11293252" deletes the file...so I'm not
sure what's up.  I can't "git rm" the file because the file isn't there.

https://codereview.chromium.org/11293252/diff/46001/webkit/tools/test_shell/t...
File webkit/tools/test_shell/test_shell.cc (right):

https://codereview.chromium.org/11293252/diff/46001/webkit/tools/test_shell/t...
webkit/tools/test_shell/test_shell.cc:172: net::URLRequestFilter* filter =
net::URLRequestFilter::GetInstance();
On 2012/12/11 19:23:34, willchan wrote:
> Whoa! What's this global here? I've never seen this in all my years on the
team.
> Let's kill this (I'm not saying you have to do this in this CL, just remarking
> this so our team can keep it in mind).
Ya, not only is it an icky global singleton but it also uses  the
URLRequest::Deprecated::RegisterProtocolFactory() interface...  Erik and I had
already planned on changing it to be a ProtocolHandler or something similar in
the long-term of this refactoring project; a change which would likely
necessitate it not being a global singleton.

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

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/net/connec...
File chrome/browser/net/connect_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/net/connec...
chrome/browser/net/connect_interceptor.cc:42: net::URLRequestJob*
ConnectInterceptor::MaybeCreateJob(
On 2012/12/11 19:14:38, willchan wrote:
> 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.
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]

Sorry, forgot to send this out yesterday.

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/net/connec...
File chrome/browser/net/connect_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/net/connec...
chrome/browser/net/connect_interceptor.cc:42: net::URLRequestJob*
ConnectInterceptor::MaybeCreateJob(
On 2012/12/11 22:28:14, pauljensen wrote:
> On 2012/12/11 19:14:38, willchan wrote:
> > 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.
> 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.

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

https://codereview.chromium.org/11293252/diff/55002/android_webview/native/an...
File android_webview/native/android_protocol_handler.h (right):

https://codereview.chromium.org/11293252/diff/55002/android_webview/native/an...
android_webview/native/android_protocol_handler.h:31: bool
RegisterAndroidProtocolHandler(JNIEnv* env);
nit:  While you're here, mind adding a line break here?

https://codereview.chromium.org/11293252/diff/55002/chrome/browser/extensions...
File chrome/browser/extensions/user_script_listener_unittest.cc (right):

https://codereview.chromium.org/11293252/diff/55002/chrome/browser/extensions...
chrome/browser/extensions/user_script_listener_unittest.cc:138: net::URLRequest*
request, net::NetworkDelegate* network_delegate) const {
nit:  OVERRIDE

https://codereview.chromium.org/11293252/diff/55002/chrome/browser/policy/dev...
File chrome/browser/policy/device_management_service_browsertest.cc (right):

https://codereview.chromium.org/11293252/diff/55002/chrome/browser/policy/dev...
chrome/browser/policy/device_management_service_browsertest.cc:57: }
nit:  Blank line.

https://codereview.chromium.org/11293252/diff/55002/chrome/browser/policy/dev...
chrome/browser/policy/device_management_service_browsertest.cc:67:
em::DeviceManagementRequest dm_request;
This seems to be left over from a refactoring.  Think we can safely delete it -
it's not used, and has no side effects.

https://codereview.chromium.org/11293252/diff/55002/chrome/browser/policy/pol...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/11293252/diff/55002/chrome/browser/policy/pol...
chrome/browser/policy/policy_browsertest.cc:138: "http://bbbb.com/empty.html",
What's the reason for this?  Other tests also using these URLs?

https://codereview.chromium.org/11293252/diff/55002/chrome/browser/printing/p...
File chrome/browser/printing/print_dialog_cloud_interative_uitest.cc (right):

https://codereview.chromium.org/11293252/diff/55002/chrome/browser/printing/p...
chrome/browser/printing/print_dialog_cloud_interative_uitest.cc:223: void
Register() {
Think that, out of a combination paranoia and desire for consistency, this
should be static, too.

https://codereview.chromium.org/11293252/diff/55002/content/test/net/url_requ...
File content/test/net/url_request_prepackaged_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/55002/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:25: }
nit:  Suggest a line break here.

https://codereview.chromium.org/11293252/diff/55002/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:40:
base::ThreadRestrictions::ScopedAllowIO allow_io;
nit:  Suggest doing this just before PathExists, to make it clearer that that's
the only line that needs it.

https://codereview.chromium.org/11293252/diff/55002/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:92: typedef
std::map<GURL, FilePath> ResponseMap;
nit:  Private typedefs should go before private methods, per google style guide.

https://codereview.chromium.org/11293252/diff/55002/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:118:
EXPECT_EQ("localhost", gurl.host());
Think doing this both here and in Delegate::SetResponse is overkill.  Fine with
only doing it in either of the two locations.  Here is nice in that it's done
inline, other location is nice in that it's nearer the actual use, and only has
to be done once for both cases.

https://codereview.chromium.org/11293252/diff/55002/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:118:
EXPECT_EQ("localhost", gurl.host());
optional:  May want to do a CHECK instead of an EXPECT.  CHECK is often used to
indicate bugs in the test fixture, rather than in the code being tested.  In
this case, it'll also prevent a potential hang.  Downside, of course, is that it
will prevent other tests from running in unit tests.

https://codereview.chromium.org/11293252/diff/55002/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:130:
EXPECT_EQ("localhost", gurl.host());
Same comment as above.

https://codereview.chromium.org/11293252/diff/55002/content/test/net/url_requ...
File content/test/net/url_request_prepackaged_interceptor.h (right):

https://codereview.chromium.org/11293252/diff/55002/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.h:31: void
SetResponseIgnoreQuery(const std::string& url, const FilePath& path);
Think both these functions should be taking GURLs, rather than strings.  Same
goes for the Delegate equivalent.

https://codereview.chromium.org/11293252/diff/55002/net/proxy/proxy_script_fe...
File net/proxy/proxy_script_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/11293252/diff/55002/net/proxy/proxy_script_fe...
net/proxy/proxy_script_fetcher_impl_unittest.cc:51: public
URLRequestJobFactory::ProtocolHandler {
Should we get rid of this, in favor of doing the check in
BasicNetworkDelegate::OnBeforeURLRequest?

https://codereview.chromium.org/11293252/diff/55002/net/url_request/url_reque...
File net/url_request/url_request_filter.cc (right):

https://codereview.chromium.org/11293252/diff/55002/net/url_request/url_reque...
net/url_request/url_request_filter.cc:27: URLRequestFilter::~URLRequestFilter()
{}
Once we can safely do it, may want to CHECK that there aren't any handlers left.
 Know the plan is to go away, but think it'd be a reasonable protection against
regressions in the mean time.

https://codereview.chromium.org/11293252/diff/55002/net/url_request/url_reque...
net/url_request/url_request_filter.cc:61:
DCHECK(hostname_handler_map_.count(make_pair(scheme, hostname)) == 0);
DCHECK_EQ(0u, ...) is preferred

https://codereview.chromium.org/11293252/diff/55002/net/url_request/url_reque...
net/url_request/url_request_filter.cc:99:
DCHECK(url_handler_map_.count(url.spec()) == 0);
DCHECK_EQ(0u, ...) is preferred

[pauljensen, 2012-12-13 17:53:29]

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/net/connec...
File chrome/browser/net/connect_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/net/connec...
chrome/browser/net/connect_interceptor.cc:42: net::URLRequestJob*
ConnectInterceptor::MaybeCreateJob(
On 2012/12/13 16:12:04, Matt Menke wrote:
> On 2012/12/11 22:28:14, pauljensen wrote:
> > On 2012/12/11 19:14:38, willchan wrote:
> > > 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.
> > 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.
> 
> 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).
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]

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/net/connec...
File chrome/browser/net/connect_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/net/connec...
chrome/browser/net/connect_interceptor.cc:42: net::URLRequestJob*
ConnectInterceptor::MaybeCreateJob(
On 2012/12/13 17:53:30, pauljensen wrote:
> On 2012/12/13 16:12:04, Matt Menke wrote:
> > On 2012/12/11 22:28:14, pauljensen wrote:
> > > On 2012/12/11 19:14:38, willchan wrote:
> > > > 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.
> > > 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.
> > 
> > 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).
> 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?
> 

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]

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/net/connec...
File chrome/browser/net/connect_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/46001/chrome/browser/net/connec...
chrome/browser/net/connect_interceptor.cc:42: net::URLRequestJob*
ConnectInterceptor::MaybeCreateJob(
On 2012/12/13 17:59:53, willchan wrote:
> On 2012/12/13 17:53:30, pauljensen wrote:
> > On 2012/12/13 16:12:04, Matt Menke wrote:
> > > On 2012/12/11 22:28:14, pauljensen wrote:
> > > > On 2012/12/11 19:14:38, willchan wrote:
> > > > > 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.
> > > > 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.
> > > 
> > > 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).
> > 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?
> > 
> 
> 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.
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.

[mmenke, 2012-12-13 18:37:10]

On 2012/12/13 18:21:54, pauljensen wrote:
> 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.

Yes, I think you can remove all those changes from this CL.  Looks to me like
there are 2 other interceptors that profiles add, so may still have to do it
later, depending on their complexity, but no need to worry about it in this CL.

[pauljensen, 2012-12-17 01:38:35]

I've addressed all comments. PTAL.

https://codereview.chromium.org/11293252/diff/55002/net/url_request/url_reque...
File net/url_request/url_request_filter.cc (right):

https://codereview.chromium.org/11293252/diff/55002/net/url_request/url_reque...
net/url_request/url_request_filter.cc:27: URLRequestFilter::~URLRequestFilter()
{}
On 2012/12/13 16:12:04, Matt Menke wrote:
> Once we can safely do it, may want to CHECK that there aren't any handlers
left.
>  Know the plan is to go away, but think it'd be a reasonable protection
against
> regressions in the mean time.
~URLRequestFilter() is never hit :'(
We could throw lots of check's in there but they won't do any good until we stop
making this a lazy singleton.  I say we punt on this until then.  Acceptable?

[pauljensen, 2012-12-17 16:26:49]

I couple responses to some past comments.

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
File content/test/net/url_request_prepackaged_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:92:
EXPECT_TRUE(file_util::ReadFileToString(response, &contents));
On 2012/12/11 17:22:38, Matt Menke wrote:
> Can we just use a "URLRequestMockHTTPJob" here?
I tried for a while to use URLRequestMockHTTPJob but this required adding lots
of *.mock-http-headers files as the update logic requires HTTP 200 response
codes.  I was fine with adding these files until it came to a new test that was
just added that creates files in a uniquely named temporary directory. 
Programmatically creating the .mock-http-headers for that test just seemed like
too much circumvention logic.  So I did what AutoUpdateInterceptor used to do,
and just made a tiny URLRequestJob class that inherited from URLRequestFileJob
and added the 200 response code.  This way the file I/O is done nicely on the
appropriate thread and we don't need the second ScopedAllowIO.

https://codereview.chromium.org/11293252/diff/55002/chrome/browser/policy/pol...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/11293252/diff/55002/chrome/browser/policy/pol...
chrome/browser/policy/policy_browsertest.cc:138: "http://bbbb.com/empty.html",
On 2012/12/13 16:12:04, Matt Menke wrote:
> What's the reason for this?  Other tests also using these URLs?
There were other tests using this but now it's unneeded so I removed this
change.

[mmenke, 2012-12-18 20:32:14]

LGTM.  Just nits.

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
File content/test/net/url_request_prepackaged_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:92:
EXPECT_TRUE(file_util::ReadFileToString(response, &contents));
On 2012/12/17 16:26:49, pauljensen wrote:
> On 2012/12/11 17:22:38, Matt Menke wrote:
> > Can we just use a "URLRequestMockHTTPJob" here?
> I tried for a while to use URLRequestMockHTTPJob but this required adding lots
> of *.mock-http-headers files as the update logic requires HTTP 200 response
> codes.  I was fine with adding these files until it came to a new test that
was
> just added that creates files in a uniquely named temporary directory. 
> Programmatically creating the .mock-http-headers for that test just seemed
like
> too much circumvention logic.  So I did what AutoUpdateInterceptor used to do,
> and just made a tiny URLRequestJob class that inherited from URLRequestFileJob
> and added the 200 response code.  This way the file I/O is done nicely on the
> appropriate thread and we don't need the second ScopedAllowIO.

Could just fix URLRequestMockHTTPJob to use a nice set of default headers.

https://codereview.chromium.org/11293252/diff/55002/net/url_request/url_reque...
File net/url_request/url_request_filter.cc (right):

https://codereview.chromium.org/11293252/diff/55002/net/url_request/url_reque...
net/url_request/url_request_filter.cc:27: URLRequestFilter::~URLRequestFilter()
{}
On 2012/12/17 01:38:35, pauljensen wrote:
> On 2012/12/13 16:12:04, Matt Menke wrote:
> > Once we can safely do it, may want to CHECK that there aren't any handlers
> left.
> >  Know the plan is to go away, but think it'd be a reasonable protection
> against
> > regressions in the mean time.
> ~URLRequestFilter() is never hit :'(
> We could throw lots of check's in there but they won't do any good until we
stop
> making this a lazy singleton.  I say we punt on this until then.  Acceptable?

SGTM.  I forgot that we leak singletons.

https://codereview.chromium.org/11293252/diff/96001/android_webview/native/an...
File android_webview/native/android_protocol_handler.cc (right):

https://codereview.chromium.org/11293252/diff/96001/android_webview/native/an...
android_webview/native/android_protocol_handler.cc:74: virtual
~AssetFileProtocolInterceptor() OVERRIDE;
nit:  Destructor should go before other methods.

https://codereview.chromium.org/11293252/diff/96001/android_webview/native/an...
android_webview/native/android_protocol_handler.cc:80:
AssetFileProtocolInterceptor();
nit:  Blank line between methods and data.

https://codereview.chromium.org/11293252/diff/96001/chrome/browser/net/chrome...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/11293252/diff/96001/chrome/browser/net/chrome...
chrome/browser/net/chrome_network_delegate.cc:404: if
(connect_interceptor_.get())
nit:  .get() not needed.

https://codereview.chromium.org/11293252/diff/96001/chrome/browser/net/connec...
File chrome/browser/net/connect_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/96001/chrome/browser/net/connec...
chrome/browser/net/connect_interceptor.cc:32: net::URLRequest* request) const {
nit:  May be able to fit on one line.

https://codereview.chromium.org/11293252/diff/96001/chrome/browser/policy/dev...
File chrome/browser/policy/device_management_service_browsertest.cc (right):

https://codereview.chromium.org/11293252/diff/96001/chrome/browser/policy/dev...
chrome/browser/policy/device_management_service_browsertest.cc:36: //
Interceptor implementation that returns test data back to the service.
Comment is no longer correct.

https://codereview.chromium.org/11293252/diff/96001/chrome/browser/printing/p...
File chrome/browser/printing/print_dialog_cloud_interative_uitest.cc (right):

https://codereview.chromium.org/11293252/diff/96001/chrome/browser/printing/p...
chrome/browser/printing/print_dialog_cloud_interative_uitest.cc:169: virtual
void SetUp() {
nit:  While you're here, these two should be OVERRIDE

https://codereview.chromium.org/11293252/diff/96001/chrome/browser/printing/p...
chrome/browser/printing/print_dialog_cloud_interative_uitest.cc:223: static void
Register(const std::string& scheme,
Think these names are a little too vague.  Just RegisterTestHandlers /
UnregisterTestHandlers would be fine, or maybe Add/Remove, to match the
underlying function names.

https://codereview.chromium.org/11293252/diff/96001/content/test/net/url_requ...
File content/test/net/url_request_prepackaged_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/96001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
Wrong year.

https://codereview.chromium.org/11293252/diff/96001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:3: // found in the
LICENSE file.
nit:  Add linebreak.

https://codereview.chromium.org/11293252/diff/96001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:82:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Since this is test code only, this DCHECK and the one above should probably be
CHECKs.

https://codereview.chromium.org/11293252/diff/96001/content/test/net/url_requ...
File content/test/net/url_request_prepackaged_interceptor.h (right):

https://codereview.chromium.org/11293252/diff/96001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.h:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
Wrong year.

https://codereview.chromium.org/11293252/diff/96001/net/url_request/protocol_...
File net/url_request/protocol_intercept_job_factory.cc (right):

https://codereview.chromium.org/11293252/diff/96001/net/url_request/protocol_...
net/url_request/protocol_intercept_job_factory.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
Hmm...Should this be 2012?  I have no idea.

[pauljensen, 2012-12-19 16:20:20]

Brett, please review the chrome/, content/ and webkit/ changes.
Jonathan, please review the android_webview/ changes.
Erik Wright and Matt Menke have thoroughly reviewed this CL as a whole.

[mmenke, 2012-12-19 16:21:21]

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
File content/test/net/url_request_prepackaged_interceptor.cc (right):

https://codereview.chromium.org/11293252/diff/46001/content/test/net/url_requ...
content/test/net/url_request_prepackaged_interceptor.cc:92:
EXPECT_TRUE(file_util::ReadFileToString(response, &contents));
On 2012/12/18 20:32:15, Matt Menke wrote:
> On 2012/12/17 16:26:49, pauljensen wrote:
> > On 2012/12/11 17:22:38, Matt Menke wrote:
> > > Can we just use a "URLRequestMockHTTPJob" here?
> > I tried for a while to use URLRequestMockHTTPJob but this required adding
lots
> > of *.mock-http-headers files as the update logic requires HTTP 200 response
> > codes.  I was fine with adding these files until it came to a new test that
> was
> > just added that creates files in a uniquely named temporary directory. 
> > Programmatically creating the .mock-http-headers for that test just seemed
> like
> > too much circumvention logic.  So I did what AutoUpdateInterceptor used to
do,
> > and just made a tiny URLRequestJob class that inherited from
URLRequestFileJob
> > and added the 200 response code.  This way the file I/O is done nicely on
the
> > appropriate thread and we don't need the second ScopedAllowIO.
> 
> Could just fix URLRequestMockHTTPJob to use a nice set of default headers.

BTW - no need to worry about that now.  Your solution is fine.  (Thought I
removed the comment).

[joth, 2012-12-19 19:20:24]

LGTM

[brettw, 2012-12-19 22:10:08]

owners lgtm rubberstamp

[commit-bot: I haz the power, 2012-12-19 22:39:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/11293252/84007

[commit-bot: I haz the power, 2012-12-19 22:40:09]

Presubmit check for 11293252-84007 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  android_webview/native/android_protocol_handler.cc:254
    static void SetResourceContextForTesting(JNIEnv* env, jclass /*clazz*/,

** Presubmit ERRORS **
Banned functions were used.
    content/test/net/url_request_prepackaged_interceptor.cc:61:
      New code should not use ScopedAllowIO. Post a task to the blocking
      pool or the FILE thread instead.

Presubmit checks took 4.5s to calculate.