Decouple URLRequestJob from URLRequestContext; access NetworkDelegate as a contructor parameter.

Decouple URLRequestJob from URLRequestContext; access NetworkDelegate as a contructor parameter.

Note: this depends on
http://codereview.chromium.org/10559036/

BUG=None
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144948

[erikwright (departed), 2012-06-13 15:44:18]

Hi Will, PTAL.

This is in support of the FtpProtocolHandler CL
(http://codereview.chromium.org/10537056/). The base URLRequestJob should
receive the NetworkDelegate from its subclass.

In the first step, this is accomplished by pushing the access to
request->context()->network_delegate() down to subclasses, but as each subclass
is converted they will themselves receive a constructor parameter and forward it
to the URLRequestJob.

http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_ab...
File net/url_request/url_request_about_job.cc (right):

http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_ab...
net/url_request/url_request_about_job.cc:19: class NetworkDelegate;
Is this forward-decl required?

[willchan no longer on Chromium, 2012-06-13 19:07:33]

Can you tell me what the plan is going forward? Are you going to gradually
change each URLRequestJob derived class to take the NetworkDelegate and its
other parameters in the derived class's constructor?

http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_jo...
File net/url_request/url_request_job.cc (left):

http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_jo...
net/url_request/url_request_job.cc:698: if (context &&
context->network_delegate())
Is this safe? We're losing the NULL check on |context| in your new code. I don't
know if there's actually a case where context is NULL. Can you hit the
tryservers with this changelist?

[erikwright (departed), 2012-06-13 19:09:17]

On 2012/06/13 19:07:33, willchan wrote:
> Can you tell me what the plan is going forward? Are you going to gradually
> change each URLRequestJob derived class to take the NetworkDelegate and its
> other parameters in the derived class's constructor?

Yes.

>
http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_jo...
> File net/url_request/url_request_job.cc (left):
> 
>
http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_jo...
> net/url_request/url_request_job.cc:698: if (context &&
> context->network_delegate())
> Is this safe? We're losing the NULL check on |context| in your new code. I
don't
> know if there's actually a case where context is NULL. Can you hit the
> tryservers with this changelist?

[willchan no longer on Chromium, 2012-06-13 19:41:50]

On 2012/06/13 15:44:18, erikwright wrote:
> Hi Will, PTAL.
> 
> This is in support of the FtpProtocolHandler CL
> (http://codereview.chromium.org/10537056/). The base URLRequestJob should
> receive the NetworkDelegate from its subclass.
> 
> In the first step, this is accomplished by pushing the access to
> request->context()->network_delegate() down to subclasses, but as each
subclass
> is converted they will themselves receive a constructor parameter and forward
it
> to the URLRequestJob.
>

Oops, sorry, I totally missed this original message :P My bad.

What do you think about just going directly to this step? I don't think it'd add
that much more work. The reason I like that is because right now, there are a
lot of extra #includes that are getting added in the subclasses for
url_request.h and url_request_context.h, and I believe those can go away if we
just go direct to exposing NetworkDelegate* in the constructor arguments.

That said, if you simply want to make progress in the current state. I'm also
cool with that. I just want to warn you to be careful about remember to remove
those includes later on.
 
>
http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_ab...
> File net/url_request/url_request_about_job.cc (right):
> 
>
http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_ab...
> net/url_request/url_request_about_job.cc:19: class NetworkDelegate;
> Is this forward-decl required?

[erikwright (departed), 2012-06-13 19:48:16]

Off the top of my head that could potentially affect a lot more files, no?

If the constructor's client is a static factory or protocol handler in the
same file it will still need the new include, and if it is not in the same
file it will affect more files.

I prefer to stick with this for now and then have more small CLs on a
per-protocol basis vs. having this one become larger.

On Wed, Jun 13, 2012 at 3:41 PM, <willchan@chromium.org> wrote:

> On 2012/06/13 15:44:18, erikwright wrote:
>
>> Hi Will, PTAL.
>>
>
>  This is in support of the FtpProtocolHandler CL
>>
(http://codereview.chromium.**org/10537056/<http://codereview.chromium.org/105...).
>> The base URLRequestJob should
>> receive the NetworkDelegate from its subclass.
>>
>
>  In the first step, this is accomplished by pushing the access to
>> request->context()->network_**delegate() down to subclasses, but as each
>>
> subclass
>
>> is converted they will themselves receive a constructor parameter and
>> forward
>>
> it
>
>> to the URLRequestJob.
>>
>
>
> Oops, sorry, I totally missed this original message :P My bad.
>
> What do you think about just going directly to this step? I don't think
> it'd add
> that much more work. The reason I like that is because right now, there
> are a
> lot of extra #includes that are getting added in the subclasses for
> url_request.h and url_request_context.h, and I believe those can go away
> if we
> just go direct to exposing NetworkDelegate* in the constructor arguments.
>
> That said, if you simply want to make progress in the current state. I'm
> also
> cool with that. I just want to warn you to be careful about remember to
> remove
> those includes later on.
>
>
>
> http://codereview.chromium.**org/10534100/diff/1/net/url_**
>
request/url_request_about_job.**cc<http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_about_job.cc>
>
>> File net/url_request/url_request_**about_job.cc (right):
>>
>
>
> http://codereview.chromium.**org/10534100/diff/1/net/url_**
>
request/url_request_about_job.**cc#newcode19<http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_about_job.cc#newcode19>
>
>> net/url_request/url_request_**about_job.cc:19: class NetworkDelegate;
>> Is this forward-decl required?
>>
>
>
>
>
http://codereview.chromium.**org/10534100/<http://codereview.chromium.org/105...
>

[erikwright (departed), 2012-06-13 19:56:47]

On 2012/06/13 19:07:33, willchan wrote:
> Can you tell me what the plan is going forward? Are you going to gradually
> change each URLRequestJob derived class to take the NetworkDelegate and its
> other parameters in the derived class's constructor?
> 
>
http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_jo...
> File net/url_request/url_request_job.cc (left):
> 
>
http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_jo...
> net/url_request/url_request_job.cc:698: if (context &&
> context->network_delegate())
> Is this safe? We're losing the NULL check on |context| in your new code. I
don't
> know if there's actually a case where context is NULL. Can you hit the
> tryservers with this changelist?

Hm. Yes, it's unfortunate that a request can possibly have a NULL context. I'd
be surprised if they were ever used that way, but perhaps they are.

If so, or even just for correctness' sake, there would be three options:
1) replace 'context()->network_delegate()' with 'context() ?
context()->network_delegate() : NULL' in all the initializer lists (ugh).
2) define URLRequest::network_delegate() that does the check
3) Actually take the NetworkDelegate as a parameter as you previously suggested.

I think I prefer (2), and would probably do it regardless of whether there are
actual uses of URLRequest with NULL context. You?

[willchan no longer on Chromium, 2012-06-13 21:55:01]

On 2012/06/13 19:56:47, erikwright wrote:
> On 2012/06/13 19:07:33, willchan wrote:
> > Can you tell me what the plan is going forward? Are you going to gradually
> > change each URLRequestJob derived class to take the NetworkDelegate and its
> > other parameters in the derived class's constructor?
> > 
> >
>
http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_jo...
> > File net/url_request/url_request_job.cc (left):
> > 
> >
>
http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_jo...
> > net/url_request/url_request_job.cc:698: if (context &&
> > context->network_delegate())
> > Is this safe? We're losing the NULL check on |context| in your new code. I
> don't
> > know if there's actually a case where context is NULL. Can you hit the
> > tryservers with this changelist?
> 
> Hm. Yes, it's unfortunate that a request can possibly have a NULL context. I'd
> be surprised if they were ever used that way, but perhaps they are.

I agree. IMO, we should fix it so the URLRequest takes the context in the
constructor. This would require updating a bunch of tests, but would be
worthwhile.

> 
> If so, or even just for correctness' sake, there would be three options:
> 1) replace 'context()->network_delegate()' with 'context() ?
> context()->network_delegate() : NULL' in all the initializer lists (ugh).
> 2) define URLRequest::network_delegate() that does the check
> 3) Actually take the NetworkDelegate as a parameter as you previously
suggested.
> 
> I think I prefer (2), and would probably do it regardless of whether there are
> actual uses of URLRequest with NULL context. You?

This is fine, although long-term I'd rather use make URLRequestContext and
NetworkDelegate required (not allowed to be NULL, since I hate wondering if we
need to do NULL checks). And longer-term, URLRequestContext may just go away.

[willchan no longer on Chromium, 2012-06-13 22:13:24]

On 2012/06/13 19:48:16, erikwright wrote:
> Off the top of my head that could potentially affect a lot more files, no?
> 
> If the constructor's client is a static factory or protocol handler in the
> same file it will still need the new include, and if it is not in the same
> file it will affect more files.

I don't think there are *that* many job factories, and I bet most of them aren't
in a different file from the job. But in that minority of cases, when we
transition the NetworkDelegate to being passed into the derived job's
constructor, then we may forget to remove the old unnecessary #include.

This is possibly premature optimization. I'm fine with whatever plan you have.
Just wanted to point this out so you don't forget.

> 
> I prefer to stick with this for now and then have more small CLs on a
> per-protocol basis vs. having this one become larger.
> 
> On Wed, Jun 13, 2012 at 3:41 PM, <mailto:willchan@chromium.org> wrote:
> 
> > On 2012/06/13 15:44:18, erikwright wrote:
> >
> >> Hi Will, PTAL.
> >>
> >
> >  This is in support of the FtpProtocolHandler CL
> >>
>
(http://codereview.chromium.**org/10537056/%3Chttp://codereview.chromium.org/1...>).
> >> The base URLRequestJob should
> >> receive the NetworkDelegate from its subclass.
> >>
> >
> >  In the first step, this is accomplished by pushing the access to
> >> request->context()->network_**delegate() down to subclasses, but as each
> >>
> > subclass
> >
> >> is converted they will themselves receive a constructor parameter and
> >> forward
> >>
> > it
> >
> >> to the URLRequestJob.
> >>
> >
> >
> > Oops, sorry, I totally missed this original message :P My bad.
> >
> > What do you think about just going directly to this step? I don't think
> > it'd add
> > that much more work. The reason I like that is because right now, there
> > are a
> > lot of extra #includes that are getting added in the subclasses for
> > url_request.h and url_request_context.h, and I believe those can go away
> > if we
> > just go direct to exposing NetworkDelegate* in the constructor arguments.
> >
> > That said, if you simply want to make progress in the current state. I'm
> > also
> > cool with that. I just want to warn you to be careful about remember to
> > remove
> > those includes later on.
> >
> >
> >
> > http://codereview.chromium.**org/10534100/diff/1/net/url_**
> >
>
request/url_request_about_job.**cc<http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_about_job.cc>
> >
> >> File net/url_request/url_request_**about_job.cc (right):
> >>
> >
> >
> > http://codereview.chromium.**org/10534100/diff/1/net/url_**
> >
>
request/url_request_about_job.**cc#newcode19<http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_about_job.cc#newcode19>
> >
> >> net/url_request/url_request_**about_job.cc:19: class NetworkDelegate;
> >> Is this forward-decl required?
> >>
> >
> >
> >
> >
>
http://codereview.chromium.**org/10534100/%3Chttp://codereview.chromium.org/1...>
> >

[shalev, 2012-06-21 20:03:55]

http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_ab...
File net/url_request/url_request_about_job.cc (right):

http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_ab...
net/url_request/url_request_about_job.cc:19: class NetworkDelegate;
On 2012/06/13 15:44:18, erikwright wrote:
> Is this forward-decl required?

Done.

http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_jo...
File net/url_request/url_request_job.cc (left):

http://codereview.chromium.org/10534100/diff/1/net/url_request/url_request_jo...
net/url_request/url_request_job.cc:698: if (context &&
context->network_delegate())
On 2012/06/13 19:07:33, willchan wrote:
> Is this safe? We're losing the NULL check on |context| in your new code. I
don't
> know if there's actually a case where context is NULL. Can you hit the
> tryservers with this changelist?

Done.

[shalev, 2012-06-22 17:03:05]

Hi Will,

This is ready for another review.

[willchan no longer on Chromium, 2012-06-22 18:03:53]

+mmenke

I'm going to start sending all of these CLs to Matt to review. Please continue
cc'ing me on these as I care about these APIs deeply and will continue to chime
in on API level issues, but will let Matt handle all the detailed reviews and
final approvals.

[mmenke, 2012-06-22 20:23:43]

http://codereview.chromium.org/10534100/diff/18002/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

http://codereview.chromium.org/10534100/diff/18002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:46: #include
"net/url_request/url_request_context.h"
Let's include url_request as well, since we're now depending on it.

http://codereview.chromium.org/10534100/diff/18002/net/url_request/url_reques...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/10534100/diff/18002/net/url_request/url_reques...
net/url_request/url_request_job.h:16: #include "base/time.h"
While you're here, could you also remove this include?  It's no longer used in
this file.

[shalev, 2012-06-22 20:53:54]

http://codereview.chromium.org/10534100/diff/18002/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

http://codereview.chromium.org/10534100/diff/18002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:46: #include
"net/url_request/url_request_context.h"
On 2012/06/22 20:23:43, Matt Menke wrote:
> Let's include url_request as well, since we're now depending on it.

Done.

http://codereview.chromium.org/10534100/diff/18002/net/url_request/url_reques...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/10534100/diff/18002/net/url_request/url_reques...
net/url_request/url_request_job.h:16: #include "base/time.h"
On 2012/06/22 20:23:43, Matt Menke wrote:
> While you're here, could you also remove this include?  It's no longer used in
> this file.

Done.

[mmenke, 2012-06-22 20:59:44]

LGTM

[commit-bot: I haz the power, 2012-06-22 21:02:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shalev@chromium.org/10534100/26001

[commit-bot: I haz the power, 2012-06-22 21:02:32]

Presubmit check for 10534100-26001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/chromeos/gdata
    webkit/fileapi
    content/browser
    webkit/appcache
    chrome/browser/ui/webui
    webkit/blob

Presubmit checks took 2.0s to calculate.

[erikwright (departed), 2012-06-22 21:14:27]

OWNERS reviews please, from:

zelidrag@chromium.org for chrome/browser/chromeos/gdata
jhawkins@chromium.org for chrome/browser/ui/webui
brettw@chromium.org for webkit/ and content/

[brettw, 2012-06-22 22:53:56]

webkit/ & content/ LGTM

[James Hawkins, 2012-06-22 23:26:56]

webui LGTM

[satorux1, 2012-06-28 20:59:03]

gdata LGTM

[commit-bot: I haz the power, 2012-06-29 17:33:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shalev@chromium.org/10534100/36003

[commit-bot: I haz the power, 2012-06-29 19:07:35]

Change committed as 144948