Remove some calls to URLRequestContext::network_delegate().

Remove some calls to URLRequestContext::network_delegate().

One new constructor interface to TestURLRequest was added to fascilitate this
refactoring.  The four argument constructor allows requests to be supplied
both a context pointer and a network delegate pointer.  The old three argument
constructor will be removed once all existing calls use the new version.
Several unit tests in net/* were updated to use this new constructor as well.

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

[tedv, 2013-02-23 04:42:25]

Erik, can you do a once-over this code before handing it off to willchan for an
official review?

[erikwright (departed), 2013-02-25 15:07:23]

In general I can't see any advantage to shuffling this accessor to the
URLRequest object.

On the contrary, the cases in this CL can seemingly all be fixed in other ways.

https://codereview.chromium.org/12328072/diff/1/net/url_request/url_request_h...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12328072/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.cc:85:
network_delegate_(request->network_delegate()),
In the various job implementations, the change should be adapted as follows:

1) The protocol handler should take a NetworkDelegate* as a parameter, and store
it as a member.
2) The URLRequestJob subclass should take a NetworkDelegate* as a parameter, and
store it as a member.
3) If there is still a static Factory function, it should fetch the
NetworkDelegate from the context (or request) and pass it to the Job subclass's
constructor.
4) Any objects created by the job (such as the HTTPTransactionDelegateImpl)
should accept the NetworkDelegate as a constructor parameter and receive it from
the job

https://codereview.chromium.org/12328072/diff/1/net/url_request/url_request_t...
File net/url_request/url_request_throttler_entry.cc (right):

https://codereview.chromium.org/12328072/diff/1/net/url_request/url_request_t...
net/url_request/url_request_throttler_entry.cc:156: request.CanThrottle() &&
GetBackoffEntry()->ShouldRejectRequest()) {
I don't think this is right. Either we can just not call into the throttling
system if the request is not eligible for throttling, or we should pass along
the network delegate during URLRequestThrottlerManager::RegisterRequestUrl

[tedv, 2013-02-26 03:28:16]

On 2013/02/25 15:07:23, erikwright wrote:
> In general I can't see any advantage to shuffling this accessor to the
> URLRequest object.

Thanks for the feedback. I'm reworking this now with that in mind.

[erikwright (departed), 2013-02-26 14:48:44]

I spoke with Jói, author of the throttling stuff. Whether to throttle or
not can be a property of the HTTP protocol handler, which can then either
invoke the throttling manager or not.

The manager can assume that it is only invoked for eligible requests.

IIUC only the extensions contexts should have it enabled. Ajwong would know
whether that includes the app contexts too.
On Feb 25, 2013 10:28 PM, <tedv@chromium.org> wrote:

> On 2013/02/25 15:07:23, erikwright wrote:
>
>> In general I can't see any advantage to shuffling this accessor to the
>> URLRequest object.
>>
>
> Thanks for the feedback. I'm reworking this now with that in mind.
>
>
https://codereview.chromium.**org/12328072/<https://codereview.chromium.org/1...
>

[awong, 2013-02-26 18:59:19]

I don't quite understand the question, but I think it's beyond my area of
expertise since I don't know what the throttler does...  If you can give me
a little more info about when/how it is used, I may at least be able to
tell you who to bug.

-Albert


On Tue, Feb 26, 2013 at 6:48 AM, Erik Wright <erikwright@chromium.org>wrote:

> I spoke with Jói, author of the throttling stuff. Whether to throttle or
> not can be a property of the HTTP protocol handler, which can then either
> invoke the throttling manager or not.
>
> The manager can assume that it is only invoked for eligible requests.
>
> IIUC only the extensions contexts should have it enabled. Ajwong would
> know whether that includes the app contexts too.
> On Feb 25, 2013 10:28 PM, <tedv@chromium.org> wrote:
>
>> On 2013/02/25 15:07:23, erikwright wrote:
>>
>>> In general I can't see any advantage to shuffling this accessor to the
>>> URLRequest object.
>>>
>>
>> Thanks for the feedback. I'm reworking this now with that in mind.
>>
>>
https://codereview.chromium.**org/12328072/<https://codereview.chromium.org/1...
>>
>

[erikwright (departed), 2013-02-26 19:05:56]

In simpler terms, which contexts might be used by requests where
"request.first_party_for_cookies().scheme() ==
extensions::kExtensionScheme"?

These requests are currently throttled and we would like to maintain that
effect.


On Tue, Feb 26, 2013 at 1:58 PM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

> I don't quite understand the question, but I think it's beyond my area of
> expertise since I don't know what the throttler does...  If you can give me
> a little more info about when/how it is used, I may at least be able to
> tell you who to bug.
>
> -Albert
>
>
> On Tue, Feb 26, 2013 at 6:48 AM, Erik Wright <erikwright@chromium.org>wrote:
>
>> I spoke with Jói, author of the throttling stuff. Whether to throttle or
>> not can be a property of the HTTP protocol handler, which can then either
>> invoke the throttling manager or not.
>>
>> The manager can assume that it is only invoked for eligible requests.
>>
>> IIUC only the extensions contexts should have it enabled. Ajwong would
>> know whether that includes the app contexts too.
>> On Feb 25, 2013 10:28 PM, <tedv@chromium.org> wrote:
>>
>>> On 2013/02/25 15:07:23, erikwright wrote:
>>>
>>>> In general I can't see any advantage to shuffling this accessor to the
>>>> URLRequest object.
>>>>
>>>
>>> Thanks for the feedback. I'm reworking this now with that in mind.
>>>
>>>
https://codereview.chromium.**org/12328072/<https://codereview.chromium.org/1...
>>>
>>
>

[awong, 2013-02-26 19:18:24]

[ +mpcomplete ]

That helps a little, but there's still some fuzziness.  Here's what I know of
Apps

(1) kExtensionScheme is "chrome-extension:" which both Apps AND extensions use.

(2) The new AppsV2 packaged apps don't allow document.cookie (though now that I
look at it...we probably have a hole where they can still read/update them via
XMLHttpRequest. dangit)

(3) chrome-extension:// resource loads are effective local file loads

With all that, I'm not quite clear on whether or not network throttler matters
for AppsV2.  For V1 Apps, I think cookies are still available, but again...why
do we care to throttle local disk accesses?

-Albert



On 2013/02/26 19:05:56, erikwright wrote:
> In simpler terms, which contexts might be used by requests where
> "request.first_party_for_cookies().scheme() ==
> extensions::kExtensionScheme"?
> 
> These requests are currently throttled and we would like to maintain that
> effect.
> 
> 
> On Tue, Feb 26, 2013 at 1:58 PM, Albert J. Wong (王重傑)
> <ajwong@chromium.org>wrote:
> 
> > I don't quite understand the question, but I think it's beyond my area of
> > expertise since I don't know what the throttler does...  If you can give me
> > a little more info about when/how it is used, I may at least be able to
> > tell you who to bug.
> >
> > -Albert
> >
> >
> > On Tue, Feb 26, 2013 at 6:48 AM, Erik Wright <erikwright@chromium.org>wrote:
> >
> >> I spoke with Jói, author of the throttling stuff. Whether to throttle or
> >> not can be a property of the HTTP protocol handler, which can then either
> >> invoke the throttling manager or not.
> >>
> >> The manager can assume that it is only invoked for eligible requests.
> >>
> >> IIUC only the extensions contexts should have it enabled. Ajwong would
> >> know whether that includes the app contexts too.
> >> On Feb 25, 2013 10:28 PM, <mailto:tedv@chromium.org> wrote:
> >>
> >>> On 2013/02/25 15:07:23, erikwright wrote:
> >>>
> >>>> In general I can't see any advantage to shuffling this accessor to the
> >>>> URLRequest object.
> >>>>
> >>>
> >>> Thanks for the feedback. I'm reworking this now with that in mind.
> >>>
> >>>
>
https://codereview.chromium.**org/12328072/%3Chttps://codereview.chromium.org...>
> >>>
> >>
> >

[Matt Perry, 2013-02-26 19:25:54]

I don't really understand either. When you say "which contexts might be used" -
what does "contexts" refer to?

On 2013/02/26 19:18:24, awong wrote:
> [ +mpcomplete ]
> 
> That helps a little, but there's still some fuzziness.  Here's what I know of
> Apps
> 
> (1) kExtensionScheme is "chrome-extension:" which both Apps AND extensions
use.
> 
> (2) The new AppsV2 packaged apps don't allow document.cookie (though now that
I
> look at it...we probably have a hole where they can still read/update them via
> XMLHttpRequest. dangit)
> 
> (3) chrome-extension:// resource loads are effective local file loads
> 
> With all that, I'm not quite clear on whether or not network throttler matters
> for AppsV2.  For V1 Apps, I think cookies are still available, but again...why
> do we care to throttle local disk accesses?
> 
> -Albert
> 
> 
> 
> On 2013/02/26 19:05:56, erikwright wrote:
> > In simpler terms, which contexts might be used by requests where
> > "request.first_party_for_cookies().scheme() ==
> > extensions::kExtensionScheme"?
> > 
> > These requests are currently throttled and we would like to maintain that
> > effect.
> > 
> > 
> > On Tue, Feb 26, 2013 at 1:58 PM, Albert J. Wong (王重傑)
> > <ajwong@chromium.org>wrote:
> > 
> > > I don't quite understand the question, but I think it's beyond my area of
> > > expertise since I don't know what the throttler does...  If you can give
me
> > > a little more info about when/how it is used, I may at least be able to
> > > tell you who to bug.
> > >
> > > -Albert
> > >
> > >
> > > On Tue, Feb 26, 2013 at 6:48 AM, Erik Wright
<erikwright@chromium.org>wrote:
> > >
> > >> I spoke with Jói, author of the throttling stuff. Whether to throttle or
> > >> not can be a property of the HTTP protocol handler, which can then either
> > >> invoke the throttling manager or not.
> > >>
> > >> The manager can assume that it is only invoked for eligible requests.
> > >>
> > >> IIUC only the extensions contexts should have it enabled. Ajwong would
> > >> know whether that includes the app contexts too.
> > >> On Feb 25, 2013 10:28 PM, <mailto:tedv@chromium.org> wrote:
> > >>
> > >>> On 2013/02/25 15:07:23, erikwright wrote:
> > >>>
> > >>>> In general I can't see any advantage to shuffling this accessor to the
> > >>>> URLRequest object.
> > >>>>
> > >>>
> > >>> Thanks for the feedback. I'm reworking this now with that in mind.
> > >>>
> > >>>
> >
>
https://codereview.chromium.**org/12328072/%253Chttps://codereview.chromium.o...>
> > >>>
> > >>
> > >

[erikwright (departed), 2013-02-26 19:28:55]

On 2013/02/26 19:18:24, awong wrote:
> [ +mpcomplete ]
> 
> That helps a little, but there's still some fuzziness.  Here's what I know of
> Apps
> 
> (1) kExtensionScheme is "chrome-extension:" which both Apps AND extensions
use.
> 
> (2) The new AppsV2 packaged apps don't allow document.cookie (though now that
I
> look at it...we probably have a hole where they can still read/update them via
> XMLHttpRequest. dangit)
> 
> (3) chrome-extension:// resource loads are effective local file loads
> 
> With all that, I'm not quite clear on whether or not network throttler matters
> for AppsV2.  For V1 Apps, I think cookies are still available, but again...why
> do we care to throttle local disk accesses?

We aren't throttling the request to chrome-extension:. We are throttling
requests where the "first_party_for_cookies" is chrome-extension. In other
words, IIUC, these are HTTP requests that were originated by extensions.

> 
> -Albert
> 
> 
> 
> On 2013/02/26 19:05:56, erikwright wrote:
> > In simpler terms, which contexts might be used by requests where
> > "request.first_party_for_cookies().scheme() ==
> > extensions::kExtensionScheme"?
> > 
> > These requests are currently throttled and we would like to maintain that
> > effect.
> > 
> > 
> > On Tue, Feb 26, 2013 at 1:58 PM, Albert J. Wong (王重傑)
> > <ajwong@chromium.org>wrote:
> > 
> > > I don't quite understand the question, but I think it's beyond my area of
> > > expertise since I don't know what the throttler does...  If you can give
me
> > > a little more info about when/how it is used, I may at least be able to
> > > tell you who to bug.
> > >
> > > -Albert
> > >
> > >
> > > On Tue, Feb 26, 2013 at 6:48 AM, Erik Wright
<erikwright@chromium.org>wrote:
> > >
> > >> I spoke with Jói, author of the throttling stuff. Whether to throttle or
> > >> not can be a property of the HTTP protocol handler, which can then either
> > >> invoke the throttling manager or not.
> > >>
> > >> The manager can assume that it is only invoked for eligible requests.
> > >>
> > >> IIUC only the extensions contexts should have it enabled. Ajwong would
> > >> know whether that includes the app contexts too.
> > >> On Feb 25, 2013 10:28 PM, <mailto:tedv@chromium.org> wrote:
> > >>
> > >>> On 2013/02/25 15:07:23, erikwright wrote:
> > >>>
> > >>>> In general I can't see any advantage to shuffling this accessor to the
> > >>>> URLRequest object.
> > >>>>
> > >>>
> > >>> Thanks for the feedback. I'm reworking this now with that in mind.
> > >>>
> > >>>
> >
>
https://codereview.chromium.**org/12328072/%253Chttps://codereview.chromium.o...>
> > >>>
> > >>
> > >

[erikwright (departed), 2013-02-26 19:29:42]

On 2013/02/26 19:25:54, Matt Perry wrote:
> I don't really understand either. When you say "which contexts might be used"
-
> what does "contexts" refer to?

URLRequestContext instances. For example, the main_context, media_context, and
the various extensions and app-specific contexts.

> 
> On 2013/02/26 19:18:24, awong wrote:
> > [ +mpcomplete ]
> > 
> > That helps a little, but there's still some fuzziness.  Here's what I know
of
> > Apps
> > 
> > (1) kExtensionScheme is "chrome-extension:" which both Apps AND extensions
> use.
> > 
> > (2) The new AppsV2 packaged apps don't allow document.cookie (though now
that
> I
> > look at it...we probably have a hole where they can still read/update them
via
> > XMLHttpRequest. dangit)
> > 
> > (3) chrome-extension:// resource loads are effective local file loads
> > 
> > With all that, I'm not quite clear on whether or not network throttler
matters
> > for AppsV2.  For V1 Apps, I think cookies are still available, but
again...why
> > do we care to throttle local disk accesses?
> > 
> > -Albert
> > 
> > 
> > 
> > On 2013/02/26 19:05:56, erikwright wrote:
> > > In simpler terms, which contexts might be used by requests where
> > > "request.first_party_for_cookies().scheme() ==
> > > extensions::kExtensionScheme"?
> > > 
> > > These requests are currently throttled and we would like to maintain that
> > > effect.
> > > 
> > > 
> > > On Tue, Feb 26, 2013 at 1:58 PM, Albert J. Wong (王重傑)
> > > <ajwong@chromium.org>wrote:
> > > 
> > > > I don't quite understand the question, but I think it's beyond my area
of
> > > > expertise since I don't know what the throttler does...  If you can give
> me
> > > > a little more info about when/how it is used, I may at least be able to
> > > > tell you who to bug.
> > > >
> > > > -Albert
> > > >
> > > >
> > > > On Tue, Feb 26, 2013 at 6:48 AM, Erik Wright
> <erikwright@chromium.org>wrote:
> > > >
> > > >> I spoke with Jói, author of the throttling stuff. Whether to throttle
or
> > > >> not can be a property of the HTTP protocol handler, which can then
either
> > > >> invoke the throttling manager or not.
> > > >>
> > > >> The manager can assume that it is only invoked for eligible requests.
> > > >>
> > > >> IIUC only the extensions contexts should have it enabled. Ajwong would
> > > >> know whether that includes the app contexts too.
> > > >> On Feb 25, 2013 10:28 PM, <mailto:tedv@chromium.org> wrote:
> > > >>
> > > >>> On 2013/02/25 15:07:23, erikwright wrote:
> > > >>>
> > > >>>> In general I can't see any advantage to shuffling this accessor to
the
> > > >>>> URLRequest object.
> > > >>>>
> > > >>>
> > > >>> Thanks for the feedback. I'm reworking this now with that in mind.
> > > >>>
> > > >>>
> > >
> >
>
https://codereview.chromium.**org/12328072/%25253Chttps://codereview.chromium...>
> > > >>>
> > > >>
> > > >

[awong, 2013-02-26 20:07:09]

On Tue, Feb 26, 2013 at 11:28 AM, <erikwright@chromium.org> wrote:

> On 2013/02/26 19:18:24, awong wrote:
>
>> [ +mpcomplete ]
>>
>
>  That helps a little, but there's still some fuzziness.  Here's what I
>> know of
>> Apps
>>
>
>  (1) kExtensionScheme is "chrome-extension:" which both Apps AND extensions
>>
> use.
>
>  (2) The new AppsV2 packaged apps don't allow document.cookie (though now
>> that
>>
> I
>
>> look at it...we probably have a hole where they can still read/update
>> them via
>> XMLHttpRequest. dangit)
>>
>
>  (3) chrome-extension:// resource loads are effective local file loads
>>
>
>  With all that, I'm not quite clear on whether or not network throttler
>> matters
>> for AppsV2.  For V1 Apps, I think cookies are still available, but
>> again...why
>> do we care to throttle local disk accesses?
>>
>
> We aren't throttling the request to chrome-extension:. We are throttling
> requests where the "first_party_for_cookies" is chrome-extension. In other
> words, IIUC, these are HTTP requests that were originated by extensions.


Gotcha. Then I think it does matter for AppsV1 and AppsV2.  In both cases,
IIRC, you can request permissions to make XHRs to normal websites which we
would want to keep subject to the throttler.  For AppsV2 also, we allow
audio/video tags to have non-local URLs (See "External resources" here
http://developer.chrome.com/apps/app_deprecated.html), but those requests
are handled by the Media request context.

<webview> tags also make HTTP requests and only exist in apps, but in that
case first_part_for_cookies will not be a chrome-extension: scheme.

FYI, I'm starting on CLs now to combine all the various Extension
RequestContexts and Media RequestContexts into the main one. I don't think
this is going to interact badly with your work but wanted to give a heads
up that we're digging in similar parts of the code.

-Albert


>
>
>
>  -Albert
>>
>
>
>
>  On 2013/02/26 19:05:56, erikwright wrote:
>> > In simpler terms, which contexts might be used by requests where
>> > "request.first_party_for_**cookies().scheme() ==
>> > extensions::kExtensionScheme"?
>> >
>> > These requests are currently throttled and we would like to maintain
>> that
>> > effect.
>> >
>> >
>> > On Tue, Feb 26, 2013 at 1:58 PM, Albert J. Wong (王重傑)
>> > <ajwong@chromium.org>wrote:
>> >
>> > > I don't quite understand the question, but I think it's beyond my
>> area of
>> > > expertise since I don't know what the throttler does...  If you can
>> give
>>
> me
>
>> > > a little more info about when/how it is used, I may at least be able
>> to
>> > > tell you who to bug.
>> > >
>> > > -Albert
>> > >
>> > >
>> > > On Tue, Feb 26, 2013 at 6:48 AM, Erik Wright
>>
> <erikwright@chromium.org>**wrote:
>
>> > >
>> > >> I spoke with Jói, author of the throttling stuff. Whether to
>> throttle or
>> > >> not can be a property of the HTTP protocol handler, which can then
>> either
>> > >> invoke the throttling manager or not.
>> > >>
>> > >> The manager can assume that it is only invoked for eligible requests.
>> > >>
>> > >> IIUC only the extensions contexts should have it enabled. Ajwong
>> would
>> > >> know whether that includes the app contexts too.
>> > >> On Feb 25, 2013 10:28 PM, <mailto:tedv@chromium.org> wrote:
>> > >>
>> > >>> On 2013/02/25 15:07:23, erikwright wrote:
>> > >>>
>> > >>>> In general I can't see any advantage to shuffling this accessor to
>> the
>> > >>>> URLRequest object.
>> > >>>>
>> > >>>
>> > >>> Thanks for the feedback. I'm reworking this now with that in mind.
>> > >>>
>> > >>>
>> >
>>
>
> https://codereview.chromium.****org/12328072/%253Chttps://code**
> review.chromium.org/12328072/ <http://codereview.chromium.org/12328072/>>
>
>> > >>>
>> > >>
>> > >
>>
>
>
>
>
https://codereview.chromium.**org/12328072/<https://codereview.chromium.org/1...
>

[tedv, 2013-02-28 03:19:58]

I've uploaded a patch set that removes the throttler interface change, as well
as the URLRequest::network_delegate() accessor.  (The code here instead accepts
the delegate on construction and stores it.)

I'm working on the changes associated with getting the network delegate to the
throttler in a different patch, but since I already have a few other CLs waiting
on the TestURLRequest change, I'd like to push this one forward to unblock them.
 So the throttler changes will be forthcoming in a future CL.

PTAL and let me know if there are any remaining issues.

[tedv, 2013-03-05 14:19:04]

Just checking back again.  willchan or erikwright, have either of you had the
chance to go over this latest patch?

On 2013/02/28 03:19:58, tedv wrote:
> I've uploaded a patch set that removes the throttler interface change, as well
> as the URLRequest::network_delegate() accessor.  (The code here instead
accepts
> the delegate on construction and stores it.)
> 
> I'm working on the changes associated with getting the network delegate to the
> throttler in a different patch, but since I already have a few other CLs
waiting
> on the TestURLRequest change, I'd like to push this one forward to unblock
them.
>  So the throttler changes will be forthcoming in a future CL.
> 
> PTAL and let me know if there are any remaining issues.

[erikwright (departed), 2013-03-06 16:48:20]

On 2013/03/05 14:19:04, tedv wrote:
> Just checking back again.  willchan or erikwright, have either of you had the
> chance to go over this latest patch?
> 
> On 2013/02/28 03:19:58, tedv wrote:
> > I've uploaded a patch set that removes the throttler interface change, as
well
> > as the URLRequest::network_delegate() accessor.  (The code here instead
> accepts
> > the delegate on construction and stores it.)
> > 
> > I'm working on the changes associated with getting the network delegate to
the
> > throttler in a different patch, but since I already have a few other CLs
> waiting
> > on the TestURLRequest change, I'd like to push this one forward to unblock
> them.
> >  So the throttler changes will be forthcoming in a future CL.
> > 
> > PTAL and let me know if there are any remaining issues.

LGTM.

[erikwright (departed), 2013-03-06 16:49:20]

On 2013/03/06 16:48:20, erikwright wrote:
> On 2013/03/05 14:19:04, tedv wrote:
> > Just checking back again.  willchan or erikwright, have either of you had
the
> > chance to go over this latest patch?
> > 
> > On 2013/02/28 03:19:58, tedv wrote:
> > > I've uploaded a patch set that removes the throttler interface change, as
> well
> > > as the URLRequest::network_delegate() accessor.  (The code here instead
> > accepts
> > > the delegate on construction and stores it.)
> > > 
> > > I'm working on the changes associated with getting the network delegate to
> the
> > > throttler in a different patch, but since I already have a few other CLs
> > waiting
> > > on the TestURLRequest change, I'd like to push this one forward to unblock
> > them.
> > >  So the throttler changes will be forthcoming in a future CL.
> > > 
> > > PTAL and let me know if there are any remaining issues.
> 
> LGTM.

mmenke: PTAL. This removes a number of clients of
URLRequestContext::network_delegate()

[mmenke, 2013-03-06 19:03:36]

On 2013/03/06 16:49:20, erikwright wrote:
> On 2013/03/06 16:48:20, erikwright wrote:
> > On 2013/03/05 14:19:04, tedv wrote:
> > > Just checking back again.  willchan or erikwright, have either of you had
> the
> > > chance to go over this latest patch?
> > > 
> > > On 2013/02/28 03:19:58, tedv wrote:
> > > > I've uploaded a patch set that removes the throttler interface change,
as
> > well
> > > > as the URLRequest::network_delegate() accessor.  (The code here instead
> > > accepts
> > > > the delegate on construction and stores it.)
> > > > 
> > > > I'm working on the changes associated with getting the network delegate
to
> > the
> > > > throttler in a different patch, but since I already have a few other CLs
> > > waiting
> > > > on the TestURLRequest change, I'd like to push this one forward to
unblock
> > > them.
> > > >  So the throttler changes will be forthcoming in a future CL.
> > > > 
> > > > PTAL and let me know if there are any remaining issues.
> > 
> > LGTM.
> 
> mmenke: PTAL. This removes a number of clients of
> URLRequestContext::network_delegate()

Taking the day off, I'll get to this tomorrow.

[mmenke, 2013-03-07 15:45:39]

https://codereview.chromium.org/12328072/diff/24001/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12328072/diff/24001/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:83: explicit
HttpTransactionDelegateImpl(
nit:  Explicit no longer needed.

https://codereview.chromium.org/12328072/diff/24001/net/url_request/url_reque...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/12328072/diff/24001/net/url_request/url_reque...
net/url_request/url_request_job.h:309: NetworkDelegate* network_delegate_;
Let's just expose this via an accessor instead.  Subclasses don't need to be
able to modify the pointer.

https://codereview.chromium.org/12328072/diff/24001/net/url_request/url_reque...
File net/url_request/url_request_job_factory_impl_unittest.cc (right):

https://codereview.chromium.org/12328072/diff/24001/net/url_request/url_reque...
net/url_request/url_request_job_factory_impl_unittest.cc:77: TestURLRequest
request(GURL("foo://bar"), &delegate, &request_context, NULL);
The TestURLRequestDelegate does have some very basic sanity checking that we
lose by using NULL instead.  Think this is fine in this CL, but it is something
to be aware of in future CLs.

[tedv, 2013-03-07 15:58:32]

https://codereview.chromium.org/12328072/diff/24001/net/url_request/url_reque...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/12328072/diff/24001/net/url_request/url_reque...
net/url_request/url_request_job.h:309: NetworkDelegate* network_delegate_;
Makes sense; will change that.

On 2013/03/07 15:45:39, mmenke wrote:
> Let's just expose this via an accessor instead.  Subclasses don't need to be
> able to modify the pointer.

https://codereview.chromium.org/12328072/diff/24001/net/url_request/url_reque...
File net/url_request/url_request_job_factory_impl_unittest.cc (right):

https://codereview.chromium.org/12328072/diff/24001/net/url_request/url_reque...
net/url_request/url_request_job_factory_impl_unittest.cc:77: TestURLRequest
request(GURL("foo://bar"), &delegate, &request_context, NULL);
My understanding of the code is that TestURLRequestContext gets constructed with
a NULL NetworkDelegate* by default, so I don't believe this loses any sanity
checking.  Unless I missed something in TestURLRequestContext::Init()?

There are other uses of TestURLRequestContexts that use non-NULL network
delegates which I've identified in other pending CLs though.

On 2013/03/07 15:45:39, mmenke wrote:
> The TestURLRequestDelegate does have some very basic sanity checking that we
> lose by using NULL instead.  Think this is fine in this CL, but it is
something
> to be aware of in future CLs.

[mmenke, 2013-03-07 16:05:58]

https://codereview.chromium.org/12328072/diff/24001/net/url_request/url_reque...
File net/url_request/url_request_job_factory_impl_unittest.cc (right):

https://codereview.chromium.org/12328072/diff/24001/net/url_request/url_reque...
net/url_request/url_request_job_factory_impl_unittest.cc:77: TestURLRequest
request(GURL("foo://bar"), &delegate, &request_context, NULL);
On 2013/03/07 15:58:32, tedv wrote:
> My understanding of the code is that TestURLRequestContext gets constructed
with
> a NULL NetworkDelegate* by default, so I don't believe this loses any sanity
> checking.  Unless I missed something in TestURLRequestContext::Init()?
> 
> There are other uses of TestURLRequestContexts that use non-NULL network
> delegates which I've identified in other pending CLs though.
> 
> On 2013/03/07 15:45:39, mmenke wrote:
> > The TestURLRequestDelegate does have some very basic sanity checking that we
> > lose by using NULL instead.  Think this is fine in this CL, but it is
> something
> > to be aware of in future CLs.
> 

It gets constructed, but if it's not passed to the URLRequest on construction
(You're passing NULL here), the request won't use it.

[tedv, 2013-03-07 21:09:26]

On 2013/03/07 16:05:58, mmenke wrote:
> It gets constructed, but if it's not passed to the URLRequest on construction
> (You're passing NULL here), the request won't use it.

Where is the NetworkDelegate constructed in this case?  My intention was
certainly to preserve the current functionality.  Looking through the call stack
for the construction of TestURLRequestContext, I see:

- URLRequestContext default constructor initializes network_delegate_ to NULL.
- TestURLRequestContext constructs, then calls TestURLRequestContext::Init().
- Init() does a bunch of stuff, but only reads network_delegate_.  It never
calls set_network_delegate(), or directly modifies it.
- Control passes back to the unit test code and (at least in this case), the
context's network delegate is never updated before creating a TestURLRequest
with that context.

How can this TestURLRequestContext be given a non-NULL network delegate?

[mmenke, 2013-03-07 21:15:15]

On 2013/03/07 21:09:26, tedv wrote:
> On 2013/03/07 16:05:58, mmenke wrote:
> > It gets constructed, but if it's not passed to the URLRequest on
construction
> > (You're passing NULL here), the request won't use it.
> 
> Where is the NetworkDelegate constructed in this case?  My intention was
> certainly to preserve the current functionality.  Looking through the call
stack
> for the construction of TestURLRequestContext, I see:
> 
> - URLRequestContext default constructor initializes network_delegate_ to NULL.
> - TestURLRequestContext constructs, then calls TestURLRequestContext::Init().
> - Init() does a bunch of stuff, but only reads network_delegate_.  It never
> calls set_network_delegate(), or directly modifies it.
> - Control passes back to the unit test code and (at least in this case), the
> context's network delegate is never updated before creating a TestURLRequest
> with that context.
> 
> How can this TestURLRequestContext be given a non-NULL network delegate?

Gah, I'm sorry.  I had been thinking the TestURLRequestContext by default
constructed a TestNetworkDelegate, and so explicitly passing NULL to
TestURLRequests would change things.  However, I was wrong about that.

[tedv, 2013-03-07 21:19:57]

On 2013/03/07 21:15:15, mmenke wrote:
> Gah, I'm sorry.  I had been thinking the TestURLRequestContext by default
> constructed a TestNetworkDelegate, and so explicitly passing NULL to
> TestURLRequests would change things.  However, I was wrong about that.

Since I'm going to modify every TestURLRequest construction anyway, would you
like me supply a TestNetworkDelegate whenever the delegate would otherwise be
NULL?  This will affect on the order of a dozen different unit test files.

[mmenke, 2013-03-07 21:23:13]

On 2013/03/07 21:19:57, tedv wrote:
> On 2013/03/07 21:15:15, mmenke wrote:
> > Gah, I'm sorry.  I had been thinking the TestURLRequestContext by default
> > constructed a TestNetworkDelegate, and so explicitly passing NULL to
> > TestURLRequests would change things.  However, I was wrong about that.
> 
> Since I'm going to modify every TestURLRequest construction anyway, would you
> like me supply a TestNetworkDelegate whenever the delegate would otherwise be
> NULL?  This will affect on the order of a dozen different unit test files.

I don't think there's currently a need for this.

[tedv, 2013-03-08 04:18:16]

I added a protected access network_delegate() method to URLRequestJob.  PTAL
when you get the chance.

[mmenke, 2013-03-08 15:42:37]

LGTM!

[commit-bot: I haz the power, 2013-03-08 15:55:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedv@chromium.org/12328072/40001

[commit-bot: I haz the power, 2013-03-08 18:04:43]

Change committed as 186982