Add HttpProtocolHandler and convert everything to use it

Add HttpProtocolHandler and convert everything to use it

Previously, http/https/ws/wss URLRequestJobs were created directly by
URLRequestJobManager, while all other URLRequestJobs were created by
URLRequestJobFactory with a relevant ProtocolHandler. This change adds an
HttpProtocolHandler and removes the special handling from URLRequestJobManager.
It allows use cases where not all of these schemes are supported, and clears
the way for removing URLRequestJobManager entirely.

Currently, one HttpProtocolHandler is added for each scheme. Having one for
multiple schemes would require changes to the way URLRequestJobFactoryImpl
stores its protocol handlers.

BUG=142945, 146591, 488166

[mgersh, 2016-04-21 17:33:08]

Description was changed from

==========
Protocol handler WIP

BUG=
==========

to

==========
Add HttpProtocolHandler and convert everything to use it

Previously, http/https/ws/wss URLRequestJobs were created directly by
URLRequestJobManager, while all other URLRequestJobs were created by
URLRequestJobFactory with a relevant ProtocolHandler. This change adds an
HttpProtocolHandler and removes the special handling from URLRequestJobManager.
It allows use cases where not all of these schemes are supported, and clears
the way for removing URLRequestJobManager entirely.

Currently, one HttpProtocolHandler is added for each scheme. Having one for
multiple schemes would require changes to the way URLRequestJobFactoryImpl
stores its protocol handlers.

BUG=142945, 146591, 488166
==========

[mgersh, 2016-04-21 17:50:41]

mgersh@chromium.org changed reviewers:
+ mmenke@chromium.org

[mgersh, 2016-04-21 18:01:55]

PTAL. This depends on the other one I just sent. I'll add other owners later.

[mmenke, 2016-04-21 20:58:01]

Preliminary comments.  Plan to do a full pass tomorrow, which I'll do regardless
of whether you upload a new CL in response to these comments by then or not.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/http_p...
File net/url_request/http_protocol_handler.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/http_p...
net/url_request/http_protocol_handler.cc:20: return
URLRequestHttpJob::Factory(request, network_delegate,
I think we should get rid of URLRequestHttpJob::Factory, and make this take its
place.  I'm not sure how much extra work that would be - may make sense to do
that in another CL.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
File net/url_request/url_request_job_factory_impl.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_job_factory_impl.cc:31:
job_factory->SetProtocolHandler("http",
Maybe use the constants in url/url_constants instead?  Doesn't really matter too
much, but does help protect against typos.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_job_factory_impl.cc:43: return
base::WrapUnique(job_factory);
Better to make job_factory a unique_ptr, and then just return it directly here. 
Should use raw pointers as rarely as you reasonably can.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
File net/url_request/url_request_job_factory_impl.h (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_job_factory_impl.h:27:
CreateWithDefaultProtocolHandlers();
Think this needs a comment mentioning what schemes this adds support for.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_job_factory_impl.h:27:
CreateWithDefaultProtocolHandlers();
Maybe rename with CreateWithHttpProtocolHandlers?  That does make it harder to
change its behavior, but I don't think we want to change what the defaults are,
anyways.  ws/wss support doesn't become any clearer, but given we don't support
it on iOS, don't think we can improve the name to make that clearer.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
File net/url_request/url_request_test_util.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_test_util.cc:663: return
http_protocol_handler_->MaybeCreateJob(request, network_delegate);
How hard would it be to make TestJobInterceptor a URLRequestJobInterceptor
instead?  Then it would just sit on top of a URLRequestJobFactoryImp that
handled HTTP itself, instead.  I think only 3 or 4 test fixtures us this class?

[mmenke, 2016-04-22 18:37:53]

LGTM

https://codereview.chromium.org/1888963004/diff/160001/android_webview/browse...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.cc:118:
DCHECK(set_protocol);
Maybe make AwURLRequestJobFactory::AwURLRequestJobFactory use
CreateWithDefaultProtocolHandlers()?

https://codereview.chromium.org/1888963004/diff/160001/android_webview/browse...
File android_webview/browser/net/aw_url_request_job_factory.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/android_webview/browse...
android_webview/browser/net/aw_url_request_job_factory.cc:44: return job;
This will switch some failures from ERR_FAILED to ERR_UNKNOWN_URL_SCHEME, at
least until you move all creation down into URLRequestFactoryImpl.  I don't
think we really care, though.

https://codereview.chromium.org/1888963004/diff/160001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:356:
net::URLRequestJobFactoryImpl::CreateWithDefaultProtocolHandlers();
Could you add a TODO about fixing this leak?

https://codereview.chromium.org/1888963004/diff/160001/content/browser/loader...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/content/browser/loader...
content/browser/loader/resource_loader_unittest.cc:594: "https",
base::WrapUnique(new net::HttpProtocolHandler));
Could you just create the factory with the defaults?  You've have to switch it
to a unique_ptr, but shouldn't be a big deal.

https://codereview.chromium.org/1888963004/diff/160001/ios/crnet/crnet_enviro...
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1888963004/diff/160001/ios/crnet/crnet_enviro...
ios/crnet/crnet_environment.mm:479:
main_context_->set_job_factory(job_factory.release());
Please add a TODO about fixing this leak

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:721: job_factory_ =
base::WrapUnique(job_factory_impl_);
Anything we do here is a bit funky, but rather than release + WrapUnique, let's
just put a std::unique_ptr<URLRequestJobFactoryImpl> on the stack, set
job_factory_impl_, and then move it over to job_factory_.

[mmenke, 2016-04-22 18:38:45]

Oops...  NOT LGTM - forgot about my first set of comments.  Like to take another
look at the code after you address them.

[mgersh, 2016-04-22 20:27:15]

https://codereview.chromium.org/1888963004/diff/160001/android_webview/browse...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.cc:118:
DCHECK(set_protocol);
On 2016/04/22 18:37:52, mmenke wrote:
> Maybe make AwURLRequestJobFactory::AwURLRequestJobFactory use
> CreateWithDefaultProtocolHandlers()?

Done.

https://codereview.chromium.org/1888963004/diff/160001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:356:
net::URLRequestJobFactoryImpl::CreateWithDefaultProtocolHandlers();
On 2016/04/22 18:37:52, mmenke wrote:
> Could you add a TODO about fixing this leak?

This function, and the crnet equivalent, already have two TODOs about this,
including one about five lines up from here, which link to a bug that applies to
the entire thing. Do you think it needs another one specifically here?

https://codereview.chromium.org/1888963004/diff/160001/content/browser/loader...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/content/browser/loader...
content/browser/loader/resource_loader_unittest.cc:594: "https",
base::WrapUnique(new net::HttpProtocolHandler));
On 2016/04/22 18:37:52, mmenke wrote:
> Could you just create the factory with the defaults?  You've have to switch it
> to a unique_ptr, but shouldn't be a big deal.

Done.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/http_p...
File net/url_request/http_protocol_handler.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/http_p...
net/url_request/http_protocol_handler.cc:20: return
URLRequestHttpJob::Factory(request, network_delegate,
On 2016/04/21 20:58:01, mmenke wrote:
> I think we should get rid of URLRequestHttpJob::Factory, and make this take
its
> place.  I'm not sure how much extra work that would be - may make sense to do
> that in another CL.

The reason why I didn't end up doing that is because the URLRequestHttpJob
constructor is protected, which I figured was so people don't try to use it and
accidentally bypass HSTS, and there are a few calls to
URLRequestHttpJob::Factory in various unit tests which I couldn't easily replace
with uses of a protocol handler and it seemed ugly to add them as friend
classes. So I'm not sure what a good way to do it would be.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
File net/url_request/url_request_job_factory_impl.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_job_factory_impl.cc:31:
job_factory->SetProtocolHandler("http",
On 2016/04/21 20:58:01, mmenke wrote:
> Maybe use the constants in url/url_constants instead?  Doesn't really matter
too
> much, but does help protect against typos.

Done.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_job_factory_impl.cc:43: return
base::WrapUnique(job_factory);
On 2016/04/21 20:58:01, mmenke wrote:
> Better to make job_factory a unique_ptr, and then just return it directly
here. 
> Should use raw pointers as rarely as you reasonably can.

Done.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
File net/url_request/url_request_job_factory_impl.h (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_job_factory_impl.h:27:
CreateWithDefaultProtocolHandlers();
On 2016/04/21 20:58:01, mmenke wrote:
> Think this needs a comment mentioning what schemes this adds support for.

Done.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_job_factory_impl.h:27:
CreateWithDefaultProtocolHandlers();
On 2016/04/21 20:58:01, mmenke wrote:
> Maybe rename with CreateWithHttpProtocolHandlers?  That does make it harder to
> change its behavior, but I don't think we want to change what the defaults
are,
> anyways.  ws/wss support doesn't become any clearer, but given we don't
support
> it on iOS, don't think we can improve the name to make that clearer.

Done.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
File net/url_request/url_request_test_util.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_test_util.cc:663: return
http_protocol_handler_->MaybeCreateJob(request, network_delegate);
On 2016/04/21 20:58:01, mmenke wrote:
> How hard would it be to make TestJobInterceptor a URLRequestJobInterceptor
> instead?  Then it would just sit on top of a URLRequestJobFactoryImp that
> handled HTTP itself, instead.  I think only 3 or 4 test fixtures us this
class?

It's enough work that I'd rather not bundle it with this CL, if that's okay with
you. The main difficulty is that I'm not seeing a convenient way to add an
interceptor without having to untangle the weird job factory structure of the
tests.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:721: job_factory_ =
base::WrapUnique(job_factory_impl_);
On 2016/04/22 18:37:53, mmenke wrote:
> Anything we do here is a bit funky, but rather than release + WrapUnique,
let's
> just put a std::unique_ptr<URLRequestJobFactoryImpl> on the stack, set
> job_factory_impl_, and then move it over to job_factory_.

Done.

[mmenke, 2016-04-22 20:41:04]

Quick responses.

https://codereview.chromium.org/1888963004/diff/160001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:356:
net::URLRequestJobFactoryImpl::CreateWithDefaultProtocolHandlers();
On 2016/04/22 20:27:15, mgersh wrote:
> On 2016/04/22 18:37:52, mmenke wrote:
> > Could you add a TODO about fixing this leak?
> 
> This function, and the crnet equivalent, already have two TODOs about this,
> including one about five lines up from here, which link to a bug that applies
to
> the entire thing. Do you think it needs another one specifically here?

I'd say yes, just to make it clearer that this is yet another instance of a
leak.  Or could update the comments about fixing leaks to make it clearer that
this is also being leaked.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/http_p...
File net/url_request/http_protocol_handler.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/http_p...
net/url_request/http_protocol_handler.cc:20: return
URLRequestHttpJob::Factory(request, network_delegate,
On 2016/04/22 20:27:15, mgersh wrote:
> On 2016/04/21 20:58:01, mmenke wrote:
> > I think we should get rid of URLRequestHttpJob::Factory, and make this take
> its
> > place.  I'm not sure how much extra work that would be - may make sense to
do
> > that in another CL.
> 
> The reason why I didn't end up doing that is because the URLRequestHttpJob
> constructor is protected, which I figured was so people don't try to use it
and
> accidentally bypass HSTS, and there are a few calls to
> URLRequestHttpJob::Factory in various unit tests which I couldn't easily
replace
> with uses of a protocol handler and it seemed ugly to add them as friend
> classes. So I'm not sure what a good way to do it would be.

I'd say it's best just to make URLRequestHttpJob's constructor.  We could make
this a public inner class of URLRequestHttpJob, but I don't think it's worth the
extra layer of obfuscation.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
File net/url_request/url_request_test_util.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_test_util.cc:663: return
http_protocol_handler_->MaybeCreateJob(request, network_delegate);
On 2016/04/22 20:27:15, mgersh wrote:
> On 2016/04/21 20:58:01, mmenke wrote:
> > How hard would it be to make TestJobInterceptor a URLRequestJobInterceptor
> > instead?  Then it would just sit on top of a URLRequestJobFactoryImp that
> > handled HTTP itself, instead.  I think only 3 or 4 test fixtures us this
> class?
> 
> It's enough work that I'd rather not bundle it with this CL, if that's okay
with
> you. The main difficulty is that I'm not seeing a convenient way to add an
> interceptor without having to untangle the weird job factory structure of the
> tests.

Unconditionally calling directly into our own private http_protocol_handler is
weird enough that I think even if the change is too much for this CL, we should
try and come up with something better in this CL.  Or we could change this to an
interceptor in a preliminay CL, before landing this one.

[mgersh, 2016-04-27 16:13:53]

https://codereview.chromium.org/1888963004/diff/160001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:356:
net::URLRequestJobFactoryImpl::CreateWithDefaultProtocolHandlers();
On 2016/04/22 20:41:04, mmenke wrote:
> On 2016/04/22 20:27:15, mgersh wrote:
> > On 2016/04/22 18:37:52, mmenke wrote:
> > > Could you add a TODO about fixing this leak?
> > 
> > This function, and the crnet equivalent, already have two TODOs about this,
> > including one about five lines up from here, which link to a bug that
applies
> to
> > the entire thing. Do you think it needs another one specifically here?
> 
> I'd say yes, just to make it clearer that this is yet another instance of a
> leak.  Or could update the comments about fixing leaks to make it clearer that
> this is also being leaked.

Done.

https://codereview.chromium.org/1888963004/diff/160001/ios/crnet/crnet_enviro...
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1888963004/diff/160001/ios/crnet/crnet_enviro...
ios/crnet/crnet_environment.mm:479:
main_context_->set_job_factory(job_factory.release());
On 2016/04/22 18:37:53, mmenke wrote:
> Please add a TODO about fixing this leak

Done.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/http_p...
File net/url_request/http_protocol_handler.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/http_p...
net/url_request/http_protocol_handler.cc:20: return
URLRequestHttpJob::Factory(request, network_delegate,
On 2016/04/22 20:41:04, mmenke wrote:
> On 2016/04/22 20:27:15, mgersh wrote:
> > On 2016/04/21 20:58:01, mmenke wrote:
> > > I think we should get rid of URLRequestHttpJob::Factory, and make this
take
> > its
> > > place.  I'm not sure how much extra work that would be - may make sense to
> do
> > > that in another CL.
> > 
> > The reason why I didn't end up doing that is because the URLRequestHttpJob
> > constructor is protected, which I figured was so people don't try to use it
> and
> > accidentally bypass HSTS, and there are a few calls to
> > URLRequestHttpJob::Factory in various unit tests which I couldn't easily
> replace
> > with uses of a protocol handler and it seemed ugly to add them as friend
> > classes. So I'm not sure what a good way to do it would be.
> 
> I'd say it's best just to make URLRequestHttpJob's constructor.  We could make
> this a public inner class of URLRequestHttpJob, but I don't think it's worth
the
> extra layer of obfuscation.

Done (assuming you meant make it public), and added a comment.

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
File net/url_request/url_request_test_util.cc (right):

https://codereview.chromium.org/1888963004/diff/160001/net/url_request/url_re...
net/url_request/url_request_test_util.cc:663: return
http_protocol_handler_->MaybeCreateJob(request, network_delegate);
On 2016/04/22 20:41:04, mmenke wrote:
> On 2016/04/22 20:27:15, mgersh wrote:
> > On 2016/04/21 20:58:01, mmenke wrote:
> > > How hard would it be to make TestJobInterceptor a URLRequestJobInterceptor
> > > instead?  Then it would just sit on top of a URLRequestJobFactoryImp that
> > > handled HTTP itself, instead.  I think only 3 or 4 test fixtures us this
> > class?
> > 
> > It's enough work that I'd rather not bundle it with this CL, if that's okay
> with
> > you. The main difficulty is that I'm not seeing a convenient way to add an
> > interceptor without having to untangle the weird job factory structure of
the
> > tests.
> 
> Unconditionally calling directly into our own private http_protocol_handler is
> weird enough that I think even if the change is too much for this CL, we
should
> try and come up with something better in this CL.  Or we could change this to
an
> interceptor in a preliminay CL, before landing this one.

Okay, I changed it to an interceptor here.

[mmenke, 2016-04-28 16:52:42]

LGTM, just nits. Thanks for doing this!

https://codereview.chromium.org/1888963004/diff/200001/content/browser/servic...
File
content/browser/service_worker/service_worker_read_from_cache_job_unittest.cc
(right):

https://codereview.chromium.org/1888963004/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_read_from_cache_job_unittest.cc:82:
std::move(test_job_factory_), base::WrapUnique(test_job_interceptor_)));
nit:  Modifying test_job_factory_ like this seems weird.

My suggestion is to just create the URLRequestJobFactoryImpl here as well,
inlining the "net::URLRequestJobFactoryImpl::CreateWithHttpProtocolHandlers()"
call.

https://codereview.chromium.org/1888963004/diff/200001/net/url_request/http_p...
File net/url_request/http_protocol_handler.cc (right):

https://codereview.chromium.org/1888963004/diff/200001/net/url_request/http_p...
net/url_request/http_protocol_handler.cc:22: request->url().scheme() == "ws" ||
request->url().scheme() == "wss");
nit:  While you're here, mind switching these to the corresponding
url_constants?

https://codereview.chromium.org/1888963004/diff/200001/net/url_request/http_p...
net/url_request/http_protocol_handler.cc:25: NOTREACHED() << "requires a valid
context";
nit:  include base/logging for NOTREACHED

https://codereview.chromium.org/1888963004/diff/200001/net/url_request/http_p...
net/url_request/http_protocol_handler.cc:27: ERR_INVALID_ARGUMENT);
nit:  Include net/base/net_error.h

https://codereview.chromium.org/1888963004/diff/200001/net/url_request/url_re...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/1888963004/diff/200001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:69:
std::move(test_job_factory_), base::WrapUnique(test_job_interceptor_)));
Suggest just inlining URLRequestJobFactoryImpl::CreateWithHttpProtocolHandlers()
here, instead of setting test_job_factory_, and then changing its value.

[mgersh, 2016-04-28 20:36:30]

Thanks for reviewing!

https://codereview.chromium.org/1888963004/diff/200001/content/browser/servic...
File
content/browser/service_worker/service_worker_read_from_cache_job_unittest.cc
(right):

https://codereview.chromium.org/1888963004/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_read_from_cache_job_unittest.cc:82:
std::move(test_job_factory_), base::WrapUnique(test_job_interceptor_)));
On 2016/04/28 16:52:41, mmenke wrote:
> nit:  Modifying test_job_factory_ like this seems weird.
> 
> My suggestion is to just create the URLRequestJobFactoryImpl here as well,
> inlining the "net::URLRequestJobFactoryImpl::CreateWithHttpProtocolHandlers()"
> call.

Done.

https://codereview.chromium.org/1888963004/diff/200001/net/url_request/http_p...
File net/url_request/http_protocol_handler.cc (right):

https://codereview.chromium.org/1888963004/diff/200001/net/url_request/http_p...
net/url_request/http_protocol_handler.cc:22: request->url().scheme() == "ws" ||
request->url().scheme() == "wss");
On 2016/04/28 16:52:41, mmenke wrote:
> nit:  While you're here, mind switching these to the corresponding
> url_constants?

Done.

https://codereview.chromium.org/1888963004/diff/200001/net/url_request/http_p...
net/url_request/http_protocol_handler.cc:25: NOTREACHED() << "requires a valid
context";
On 2016/04/28 16:52:41, mmenke wrote:
> nit:  include base/logging for NOTREACHED

Done.

https://codereview.chromium.org/1888963004/diff/200001/net/url_request/http_p...
net/url_request/http_protocol_handler.cc:27: ERR_INVALID_ARGUMENT);
On 2016/04/28 16:52:41, mmenke wrote:
> nit:  Include net/base/net_error.h

Done.

https://codereview.chromium.org/1888963004/diff/200001/net/url_request/url_re...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/1888963004/diff/200001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:69:
std::move(test_job_factory_), base::WrapUnique(test_job_interceptor_)));
On 2016/04/28 16:52:41, mmenke wrote:
> Suggest just inlining
URLRequestJobFactoryImpl::CreateWithHttpProtocolHandlers()
> here, instead of setting test_job_factory_, and then changing its value.

Done.

[mgersh, 2016-04-28 20:48:02]

mgersh@chromium.org changed reviewers:
+ creis@chromium.org, sdefresne@chromium.org, sgurun@chromium.org,
skyostil@chromium.org, slan@chromium.org

[mgersh, 2016-04-28 20:48:03]

creis: PTAL at content/
sdefresne: PTAL at components/ and ios/
sgurun: PTAL at android_webview/
skyostil: PTAL at headless/
slan: PTAL at chromecast/

Thanks!

[Charlie Reis, 2016-04-28 21:47:24]

content/ LGTM

[sdefresne, 2016-04-30 12:13:41]

components/ & ios/ lgtm

[sgurun-gerrit only, 2016-05-02 21:22:14]

On 2016/04/30 12:13:41, sdefresne wrote:
> components/ & ios/ lgtm

aw lgtm

[Sami, 2016-05-03 13:20:28]

headless/ lgtm

[slan, 2016-05-05 18:37:39]

cast lgtm

[mmenke, 2016-06-01 15:32:48]

Miriam:  What's the status of this CL?

[maksims (do not use this acc), 2016-07-29 08:29:00]

On 2016/06/01 15:32:48, mmenke wrote:
> Miriam:  What's the status of this CL?

I have the same question: what's the status of this CL?

[sgurun-gerrit only, 2016-08-22 18:26:15]

On 2016/07/29 08:29:00, maksims wrote:
> On 2016/06/01 15:32:48, mmenke wrote:
> > Miriam:  What's the status of this CL?
> 
> I have the same question: what's the status of this CL?

based on no response to questions above, I am guessing this CL won't land. Can
you please close the CL?

[alex clarke (OOO till 29th), 2016-08-23 17:54:01]

On 2016/08/22 18:26:15, sgurun wrote:
> On 2016/07/29 08:29:00, maksims wrote:
> > On 2016/06/01 15:32:48, mmenke wrote:
> > > Miriam:  What's the status of this CL?
> > 
> > I have the same question: what's the status of this CL?
> 
> based on no response to questions above, I am guessing this CL won't land. Can
> you please close the CL?

That's too bad, HttpProtocolHandler looks like it would be somewhat useful for
headless.

[mgersh, 2016-08-23 23:40:56]

On 2016/08/23 17:54:01, alex clarke wrote:
> On 2016/08/22 18:26:15, sgurun wrote:
> > On 2016/07/29 08:29:00, maksims wrote:
> > > On 2016/06/01 15:32:48, mmenke wrote:
> > > > Miriam:  What's the status of this CL?
> > > 
> > > I have the same question: what's the status of this CL?
> > 
> > based on no response to questions above, I am guessing this CL won't land.
Can
> > you please close the CL?
> 
> That's too bad, HttpProtocolHandler looks like it would be somewhat useful for
> headless.

Hi all, I'm sorry about dropping this on the floor and being unresponsive. The
current status is that it depends on another CL which had an issue. I'm now
planning to pick it back up in the next couple of weeks and get it done in time
for M55.

[sdefresne, 2017-05-03 09:09:19]

Is this CL still pending or should it be closed?

[sgurun-gerrit only, 2017-05-03 20:02:53]

On 2017/05/03 09:09:19, sdefresne wrote:
> Is this CL still pending or should it be closed?

it should be closed IMO. I am getting uncomfortable LGTMing a CL and then let it
wait for a year on the principle that things may have changed since then

[mgersh, 2017-05-03 22:14:49]

On 2017/05/03 20:02:53, sgurun wrote:
> On 2017/05/03 09:09:19, sdefresne wrote:
> > Is this CL still pending or should it be closed?
> 
> it should be closed IMO. I am getting uncomfortable LGTMing a CL and then let
it
> wait for a year on the principle that things may have changed since then

I agree that it's now too out of date to be all that useful anyway, so I'll
close it.

[mgersh, 2017-05-03 22:15:08]

Description was changed from

==========
Add HttpProtocolHandler and convert everything to use it

Previously, http/https/ws/wss URLRequestJobs were created directly by
URLRequestJobManager, while all other URLRequestJobs were created by
URLRequestJobFactory with a relevant ProtocolHandler. This change adds an
HttpProtocolHandler and removes the special handling from URLRequestJobManager.
It allows use cases where not all of these schemes are supported, and clears
the way for removing URLRequestJobManager entirely.

Currently, one HttpProtocolHandler is added for each scheme. Having one for
multiple schemes would require changes to the way URLRequestJobFactoryImpl
stores its protocol handlers.

BUG=142945, 146591, 488166
==========

to

==========
Add HttpProtocolHandler and convert everything to use it

Previously, http/https/ws/wss URLRequestJobs were created directly by
URLRequestJobManager, while all other URLRequestJobs were created by
URLRequestJobFactory with a relevant ProtocolHandler. This change adds an
HttpProtocolHandler and removes the special handling from URLRequestJobManager.
It allows use cases where not all of these schemes are supported, and clears
the way for removing URLRequestJobManager entirely.

Currently, one HttpProtocolHandler is added for each scheme. Having one for
multiple schemes would require changes to the way URLRequestJobFactoryImpl
stores its protocol handlers.

BUG=142945, 146591, 488166
==========