Replaced static URLRequestHTTPJob factory with non-static protocol handler for HTTP jobs.

Replaced static URLRequestHTTPJob factory with non-static protocol handler for HTTP jobs.

BUG=None
TEST=None

[erikwright (departed), 2012-07-17 19:42:45]

LGTM.

http://codereview.chromium.org/10702137/diff/8001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/10702137/diff/8001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:497: job_factories[1],
media_request_context_->ftp_auth_cache(),
Nit: this wrapping is non-conforming.

Put all three in the form

CreateFtpProtocolHandler(job_factories[0],
                         main_context->ftp_auth_cache(),
                         network_delegate());

http://codereview.chromium.org/10702137/diff/8001/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10702137/diff/8001/net/url_request/url_request...
net/url_request/url_request_http_job.cc:329: DCHECK(http_transaction_factory_);
Probably best to move this up to the constructor now.

http://codereview.chromium.org/10702137/diff/8001/net/url_request/url_request...
net/url_request/url_request_http_job.cc:854:
request_info_.extra_headers.SetHeaderIfMissing(
Add a TODO here:

TODO(shalev): Fix this to not go through URLRequestContext.

http://codereview.chromium.org/10702137/diff/8001/net/url_request/url_request...
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/10702137/diff/8001/net/url_request/url_request...
net/url_request/url_request_http_job.h:45: explicit URLRequestHttpJob(
-explicit

[shalev, 2012-07-17 20:20:18]

Hi Matt,

Could you take a look at this?

Thanks,

Shalev

http://codereview.chromium.org/10702137/diff/8001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/10702137/diff/8001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:497: job_factories[1],
media_request_context_->ftp_auth_cache(),
On 2012/07/17 19:42:45, erikwright wrote:
> Nit: this wrapping is non-conforming.
> 
> Put all three in the form
> 
> CreateFtpProtocolHandler(job_factories[0],
>                          main_context->ftp_auth_cache(),
>                          network_delegate());

Done.

http://codereview.chromium.org/10702137/diff/8001/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10702137/diff/8001/net/url_request/url_request...
net/url_request/url_request_http_job.cc:329: DCHECK(http_transaction_factory_);
On 2012/07/17 19:42:45, erikwright wrote:
> Probably best to move this up to the constructor now.

Done.

http://codereview.chromium.org/10702137/diff/8001/net/url_request/url_request...
net/url_request/url_request_http_job.cc:854:
request_info_.extra_headers.SetHeaderIfMissing(
On 2012/07/17 19:42:45, erikwright wrote:
> Add a TODO here:
> 
> TODO(shalev): Fix this to not go through URLRequestContext.

Done.

http://codereview.chromium.org/10702137/diff/8001/net/url_request/url_request...
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/10702137/diff/8001/net/url_request/url_request...
net/url_request/url_request_http_job.h:45: explicit URLRequestHttpJob(
On 2012/07/17 19:42:45, erikwright wrote:
> -explicit

Done.

[mmenke, 2012-07-18 16:21:53]

http://codereview.chromium.org/10702137/diff/4002/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10702137/diff/4002/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:274: NULL,
Same comment about this being NULL as in the other CL applies here and to the
other file.

http://codereview.chromium.org/10702137/diff/4002/net/url_request/http_protoc...
File net/url_request/http_protocol_handler.h (right):

http://codereview.chromium.org/10702137/diff/4002/net/url_request/http_protoc...
net/url_request/http_protocol_handler.h:7: 
#include <string>

http://codereview.chromium.org/10702137/diff/4002/net/url_request/http_protoc...
net/url_request/http_protocol_handler.h:45: std::string accept_charset_;
nit:  Might as well make both these strings const.

http://codereview.chromium.org/10702137/diff/4002/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10702137/diff/4002/net/url_request/url_request...
net/url_request/url_request_http_job.cc:155:
request->context()->transport_security_state());
nit:  4-space indent.

http://codereview.chromium.org/10702137/diff/4002/net/url_request/url_request...
net/url_request/url_request_http_job.cc:639: host, sni_available,
&domain_state)) {
nit:  Since this is a continuation of the function called on the previous line,
think indenting 4 more spaces makes this a little easier to read.  Suggest the
same in the similar line below as well.

http://codereview.chromium.org/10702137/diff/4002/net/url_request/url_request...
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/10702137/diff/4002/net/url_request/url_request...
net/url_request/url_request_http_job.h:259: std::string accept_charset_;
nit:  Can make these strings const.

[shalev, 2012-07-23 21:00:21]

http://codereview.chromium.org/10702137/diff/4002/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10702137/diff/4002/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:274: NULL,
On 2012/07/18 16:21:53, Matt Menke wrote:
> Same comment about this being NULL as in the other CL applies here and to the
> other file.

I responded to this comment in the other cl: I am trying to preserve the
previous behavior, and as far as I can tell this NetworkDelegate used to be
NULL.

http://codereview.chromium.org/10702137/diff/4002/net/url_request/http_protoc...
File net/url_request/http_protocol_handler.h (right):

http://codereview.chromium.org/10702137/diff/4002/net/url_request/http_protoc...
net/url_request/http_protocol_handler.h:7: 
On 2012/07/18 16:21:53, Matt Menke wrote:
> #include <string>

Done.

http://codereview.chromium.org/10702137/diff/4002/net/url_request/http_protoc...
net/url_request/http_protocol_handler.h:45: std::string accept_charset_;
On 2012/07/18 16:21:53, Matt Menke wrote:
> nit:  Might as well make both these strings const.

Done.

http://codereview.chromium.org/10702137/diff/4002/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10702137/diff/4002/net/url_request/url_request...
net/url_request/url_request_http_job.cc:155:
request->context()->transport_security_state());
On 2012/07/18 16:21:53, Matt Menke wrote:
> nit:  4-space indent.

Done.

http://codereview.chromium.org/10702137/diff/4002/net/url_request/url_request...
net/url_request/url_request_http_job.cc:639: host, sni_available,
&domain_state)) {
On 2012/07/18 16:21:53, Matt Menke wrote:
> nit:  Since this is a continuation of the function called on the previous
line,
> think indenting 4 more spaces makes this a little easier to read.  Suggest the
> same in the similar line below as well.

Done.

http://codereview.chromium.org/10702137/diff/4002/net/url_request/url_request...
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/10702137/diff/4002/net/url_request/url_request...
net/url_request/url_request_http_job.h:259: std::string accept_charset_;
On 2012/07/18 16:21:53, Matt Menke wrote:
> nit:  Can make these strings const.

Done.

[mmenke, 2012-07-24 15:24:34]

http://codereview.chromium.org/10702137/diff/14002/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10702137/diff/14002/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:269:
transport_security_state()));
Code looks fine to me, as-is, but I think it might be simpler to have a
function:

ProfileIOData::SetupJobFactory(job_factory, cache, network_delegate,
cookie_store, fraudulent_certificate_reporter, ftp_auth_cache);

Could eventually remove the auth cache argument, and have the FtpProtocolHandler
create/own it (Unless we want to have the media and main contexts share one).

http://codereview.chromium.org/10702137/diff/14002/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:348:
DCHECK(set_protocol);
May want to just inline these, to be consistent with the HTTP protocol handlers.

http://codereview.chromium.org/10702137/diff/14002/net/url_request/http_proto...
File net/url_request/http_protocol_handler.cc (right):

http://codereview.chromium.org/10702137/diff/14002/net/url_request/http_proto...
net/url_request/http_protocol_handler.cc:41: return new
URLRequestErrorJob(request, ERR_INVALID_ARGUMENT);
I don't believe we need this, and I don't think we will in the future, either. 
Let's just do DCHECK(http_transaction_factory_) on construction instead.  Can
also remove the header for the error job.

http://codereview.chromium.org/10702137/diff/14002/net/url_request/url_reques...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10702137/diff/14002/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:712: 
nit:  Remove one of the two blank lines.

http://codereview.chromium.org/10702137/diff/14002/net/url_request/url_reques...
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/10702137/diff/14002/net/url_request/url_reques...
net/url_request/url_request_http_job.h:40: friend class HttpProtocolHandler;
This should be at the top of the private section.

[shalev, 2012-07-25 20:22:45]

http://codereview.chromium.org/10702137/diff/14002/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10702137/diff/14002/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:269:
transport_security_state()));
On 2012/07/24 15:24:34, Matt Menke wrote:
> Code looks fine to me, as-is, but I think it might be simpler to have a
> function:
> 
> ProfileIOData::SetupJobFactory(job_factory, cache, network_delegate,
> cookie_store, fraudulent_certificate_reporter, ftp_auth_cache);
> 
> Could eventually remove the auth cache argument, and have the
FtpProtocolHandler
> create/own it (Unless we want to have the media and main contexts share one).

I think I prefer to leave it this way for now, because your suggestion doesn't
reduce the number of lines of code by much, and because we may decide in the
future that we don't want every JobFactory to have every kind of protocol
handler. If you still want me to create the SetupJobFactory function in
ProfileIOData, let me know, and I will do that.

I agree with your point about the auth cache, but that's probably best saved for
a future cl.

http://codereview.chromium.org/10702137/diff/14002/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:348:
DCHECK(set_protocol);
On 2012/07/24 15:24:34, Matt Menke wrote:
> May want to just inline these, to be consistent with the HTTP protocol
handlers.

Are you sure we want to do this? I understand that it would be more consistent
with http, but it may hurt readability. In particular, if we change
ftp_auth_cache to be owned by ftpProtocolHandler, then each call to
CreateFtpProtocolHandler will be only one line.

(Originally I wanted to have a helper function for HttpProtocolHandler as well,
but it turned out to need so many arguments that it seemed simpler to just
inline it.)

http://codereview.chromium.org/10702137/diff/14002/net/url_request/http_proto...
File net/url_request/http_protocol_handler.cc (right):

http://codereview.chromium.org/10702137/diff/14002/net/url_request/http_proto...
net/url_request/http_protocol_handler.cc:41: return new
URLRequestErrorJob(request, ERR_INVALID_ARGUMENT);
On 2012/07/24 15:24:34, Matt Menke wrote:
> I don't believe we need this, and I don't think we will in the future, either.

> Let's just do DCHECK(http_transaction_factory_) on construction instead.  Can
> also remove the header for the error job.

Done.

http://codereview.chromium.org/10702137/diff/14002/net/url_request/url_reques...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10702137/diff/14002/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:712: 
On 2012/07/24 15:24:34, Matt Menke wrote:
> nit:  Remove one of the two blank lines.

Done.

http://codereview.chromium.org/10702137/diff/14002/net/url_request/url_reques...
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/10702137/diff/14002/net/url_request/url_reques...
net/url_request/url_request_http_job.h:40: friend class HttpProtocolHandler;
On 2012/07/24 15:24:34, Matt Menke wrote:
> This should be at the top of the private section.

Done.

[mmenke, 2012-07-26 14:22:06]

LGTM

http://codereview.chromium.org/10702137/diff/14002/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10702137/diff/14002/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:269:
transport_security_state()));
On 2012/07/25 20:22:45, shalev wrote:
> On 2012/07/24 15:24:34, Matt Menke wrote:
> > Code looks fine to me, as-is, but I think it might be simpler to have a
> > function:
> > 
> > ProfileIOData::SetupJobFactory(job_factory, cache, network_delegate,
> > cookie_store, fraudulent_certificate_reporter, ftp_auth_cache);
> > 
> > Could eventually remove the auth cache argument, and have the
> FtpProtocolHandler
> > create/own it (Unless we want to have the media and main contexts share
one).
> 
> I think I prefer to leave it this way for now, because your suggestion doesn't
> reduce the number of lines of code by much, and because we may decide in the
> future that we don't want every JobFactory to have every kind of protocol
> handler. If you still want me to create the SetupJobFactory function in
> ProfileIOData, let me know, and I will do that.
> 
> I agree with your point about the auth cache, but that's probably best saved
for
> a future cl.

Works for me.

http://codereview.chromium.org/10702137/diff/14002/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:348:
DCHECK(set_protocol);
On 2012/07/25 20:22:45, shalev wrote:
> On 2012/07/24 15:24:34, Matt Menke wrote:
> > May want to just inline these, to be consistent with the HTTP protocol
> handlers.
> 
> Are you sure we want to do this? I understand that it would be more consistent
> with http, but it may hurt readability. In particular, if we change
> ftp_auth_cache to be owned by ftpProtocolHandler, then each call to
> CreateFtpProtocolHandler will be only one line.
> 
> (Originally I wanted to have a helper function for HttpProtocolHandler as
well,
> but it turned out to need so many arguments that it seemed simpler to just
> inline it.)

I just think it's a little weird to create a function that does this, saving us
3 lines of code, and not to do it for the HTTP handler (Which would save us
about the same).

The tradeoffs seem very similar in both cases:  It saves a few lines of code in
a massive function, and draws attention to where the arguments passed in to the
constructor differ, at the cost of a little more total code, making it more
difficult to work through all constructor arguments.

I don't feel that strongly about it, and am not going to block this CL over it,
I just it's weird to use differing styles for doing the same thing in the same
file.

http://codereview.chromium.org/10702137/diff/14002/net/url_request/http_proto...
File net/url_request/http_protocol_handler.cc (right):

http://codereview.chromium.org/10702137/diff/14002/net/url_request/http_proto...
net/url_request/http_protocol_handler.cc:41: return new
URLRequestErrorJob(request, ERR_INVALID_ARGUMENT);
On 2012/07/25 20:22:45, shalev wrote:
> On 2012/07/24 15:24:34, Matt Menke wrote:
> > I don't believe we need this, and I don't think we will in the future,
either.
> 
> > Let's just do DCHECK(http_transaction_factory_) on construction instead. 
Can
> > also remove the header for the error job.
> 
> Done.

The DCHECK should be in the constructor.

[shalev, 2012-07-26 16:11:48]

Hi Will,

Since you are an owner of profiles, could you give me a review of this cl? I
also need a review for the DataJob cl:
http://codereview.chromium.org/10703089/

Thanks!

Shalev

[mmenke, 2012-07-26 16:14:14]

http://codereview.chromium.org/10702137/diff/21001/net/url_request/http_proto...
File net/url_request/http_protocol_handler.cc (right):

http://codereview.chromium.org/10702137/diff/21001/net/url_request/http_proto...
net/url_request/http_protocol_handler.cc:37: DCHECK(http_transaction_factory_);
Just making sure you saw this comment, since I put it in the old patch set,
below the others:  The DCHECK should be in the constructor.

[shalev, 2012-07-26 17:45:39]

http://codereview.chromium.org/10702137/diff/14002/net/url_request/http_proto...
File net/url_request/http_protocol_handler.cc (right):

http://codereview.chromium.org/10702137/diff/14002/net/url_request/http_proto...
net/url_request/http_protocol_handler.cc:41: return new
URLRequestErrorJob(request, ERR_INVALID_ARGUMENT);
On 2012/07/26 14:22:06, Matt Menke wrote:
> On 2012/07/25 20:22:45, shalev wrote:
> > On 2012/07/24 15:24:34, Matt Menke wrote:
> > > I don't believe we need this, and I don't think we will in the future,
> either.
> > 
> > > Let's just do DCHECK(http_transaction_factory_) on construction instead. 
> Can
> > > also remove the header for the error job.
> > 
> > Done.
> 
> The DCHECK should be in the constructor.

Done.

http://codereview.chromium.org/10702137/diff/21001/net/url_request/http_proto...
File net/url_request/http_protocol_handler.cc (right):

http://codereview.chromium.org/10702137/diff/21001/net/url_request/http_proto...
net/url_request/http_protocol_handler.cc:37: DCHECK(http_transaction_factory_);
On 2012/07/26 16:14:14, Matt Menke wrote:
> Just making sure you saw this comment, since I put it in the old patch set,
> below the others:  The DCHECK should be in the constructor.

Done.

[willchan no longer on Chromium, 2012-07-27 19:12:55]

http://codereview.chromium.org/10702137/diff/29001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10702137/diff/29001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:264:
main_context->accept_language(),
I think this is a bug. accept_language and accept_charset can dynamically
change. See chrome_url_request_context.cc for how we observe preference change
notifications to adjust these values. We should not configure it only once on
startup.

If my analysis is correct, please fix this and add a regression test.