Allow URLRequests from one context to have different NetworkDelegates.

Allow URLRequests from one context to have different NetworkDelegates.

This is a prerequisite to allowing a URLRequestJob to transparently wrap
a URLRequest, so AppCache can override the response for failing and
redirected requests without the NetworkDelegate or URLRequest::Delegate
being aware of the underlying request.

Also consolidate the URLRequest constructors, and make most code create
URLRequests through the URLRequestContext's CreateRequest function,
rather than through its constructor.

TBR=battre@chromium.org
BUG=161547
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291090

[mmenke, 2014-07-24 16:24:41]

I hadn't realized this would be quite so huge...  The relevant changes are in in
url_request.*, url_request_context.*, url_request_http_job.cc, and
url_Request_throttler_entry*.

Most everything else is switching from calling new URLRequest to calling
URLRequestContext::CreateRequest, which can pass objects the context has
pointers to to the URLRequest on construction, rather than requiring the
URLRequest dig into the context.

Conveniently, this is probably something that would be needed anyways, before
converting the context into a URLRequestFactory.  Long term, it should just have
pointers to objects the URLRequest needs (job factory, cookie store, network
delegate, not sure what else) and pass them all in to the new URLRequest when
CreateRequest is called.

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
net/url_request/url_request.h:270: // from the URLRequestContext.
Not a huge fan of this behavior, but it matches what we do with cookie_store.

Eventually, should probably get rid of it, and move the behavior to the
URLRequestContext, and make the URLRequestContext members private.

We should probably get rid of TestURLRequest before making that change.

[pauljensen, 2014-07-31 15:38:17]

I'd suggest rewording "Allow different URLRequests to have different
NetworkDelegates." as I believe if you have multiple URLRequestContext's
different URLRequests can have different NetworkDelegates currently.  Maybe say
"URLRequests from the same URLRequestContext".

https://codereview.chromium.org/407093011/diff/220001/extensions/browser/exte...
File extensions/browser/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/407093011/diff/220001/extensions/browser/exte...
extensions/browser/extension_protocols_unittest.cc:328:
extension->GetResourceURL("test.dat"),
two space indent for function call is wrong

https://codereview.chromium.org/407093011/diff/220001/extensions/browser/exte...
extensions/browser/extension_protocols_unittest.cc:338:
extension->GetResourceURL("test.dat"),
ditto

https://codereview.chromium.org/407093011/diff/220001/extensions/browser/exte...
extensions/browser/extension_protocols_unittest.cc:350:
extension->GetResourceURL("test.dat"),
ditto

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
net/url_request/url_request.cc:236: DCHECK(context);
I see you lightened this from CHECK to DCHECK.  I think we should delete this as
|context| is dereferenced unconditionally above (line 209) and below (line 237).

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
net/url_request/url_request.h:270: // from the URLRequestContext.
On 2014/07/24 16:24:41, mmenke wrote:
> Not a huge fan of this behavior, but it matches what we do with cookie_store.
> 
> Eventually, should probably get rid of it, and move the behavior to the
> URLRequestContext, and make the URLRequestContext members private.
> 
> We should probably get rid of TestURLRequest before making that change.

Acknowledged.

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
net/url_request/url_request_context.cc:82: network_delegate_));
Could just pass in NULL but I guess it doesn't really make a difference.

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
File net/url_request/url_request_test_util.h (right):

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
net/url_request/url_request_test_util.h:263: size_t total_seen_requests() const
{ return event_order_.size(); }
I didn't see any caller of this function nor a use of the next two changes.  Am
I missing something?

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
File net/url_request/url_request_throttler_simulation_unittest.cc (right):

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
net/url_request/url_request_throttler_simulation_unittest.cc:279:
URLRequestContext* context() { return &context_; }
const?

[mmenke, 2014-07-31 16:03:15]

Thanks for the feedback, and for making it through the review!  Sure it was as
painful to review as it was to write, though I did write a script to handle
url_request_unittest.cc.

https://codereview.chromium.org/407093011/diff/220001/extensions/browser/exte...
File extensions/browser/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/407093011/diff/220001/extensions/browser/exte...
extensions/browser/extension_protocols_unittest.cc:328:
extension->GetResourceURL("test.dat"),
On 2014/07/31 15:38:17, pauljensen wrote:
> two space indent for function call is wrong

Done.  Also fixed the others in this file you didn't point out.

Thanks for catching these.  This CL became more than a little mind-numbing,
after a while.  :)

https://codereview.chromium.org/407093011/diff/220001/extensions/browser/exte...
extensions/browser/extension_protocols_unittest.cc:338:
extension->GetResourceURL("test.dat"),
On 2014/07/31 15:38:17, pauljensen wrote:
> ditto

Done.

https://codereview.chromium.org/407093011/diff/220001/extensions/browser/exte...
extensions/browser/extension_protocols_unittest.cc:350:
extension->GetResourceURL("test.dat"),
On 2014/07/31 15:38:17, pauljensen wrote:
> ditto

Done.

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
net/url_request/url_request.cc:236: DCHECK(context);
On 2014/07/31 15:38:17, pauljensen wrote:
> I see you lightened this from CHECK to DCHECK.  I think we should delete this
as
> |context| is dereferenced unconditionally above (line 209) and below (line
237).

Done.

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
net/url_request/url_request_context.cc:82: network_delegate_));
On 2014/07/31 15:38:17, pauljensen wrote:
> Could just pass in NULL but I guess it doesn't really make a difference.

Yea, I'm fine either way, here.  I'm keeping as-is, just because I feel that if
it's NULL, a comment about it is warranted.

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
File net/url_request/url_request_test_util.h (right):

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
net/url_request/url_request_test_util.h:263: size_t total_seen_requests() const
{ return event_order_.size(); }
On 2014/07/31 15:38:17, pauljensen wrote:
> I didn't see any caller of this function nor a use of the next two changes. 
Am
> I missing something?

No, you're not.  These accidentally slipped in from the that creates the new
wrapper job class.  I've removed these changes from this CL.

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
File net/url_request/url_request_throttler_simulation_unittest.cc (right):

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
net/url_request/url_request_throttler_simulation_unittest.cc:279:
URLRequestContext* context() { return &context_; }
On 2014/07/31 15:38:17, pauljensen wrote:
> const?

Done.  Left on my own, I never mark functions that return non-const references
to objects owned by a class const, because it seems like non-const-y behavior to
me.

[pauljensen, 2014-07-31 16:25:51]

What did you think of my commit message comment?

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
File net/url_request/url_request_throttler_simulation_unittest.cc (right):

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
net/url_request/url_request_throttler_simulation_unittest.cc:279:
URLRequestContext* context() { return &context_; }
On 2014/07/31 16:03:14, mmenke wrote:
> On 2014/07/31 15:38:17, pauljensen wrote:
> > const?
> 
> Done.  Left on my own, I never mark functions that return non-const references
> to objects owned by a class const, because it seems like non-const-y behavior
to
> me.
The return type could be a const reference.

[mmenke, 2014-07-31 19:02:12]

Updated the description as well (Noticed your comment about it earlier, but then
forgot to update it).

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
File net/url_request/url_request_throttler_simulation_unittest.cc (right):

https://codereview.chromium.org/407093011/diff/220001/net/url_request/url_req...
net/url_request/url_request_throttler_simulation_unittest.cc:279:
URLRequestContext* context() { return &context_; }
On 2014/07/31 16:25:51, pauljensen wrote:
> On 2014/07/31 16:03:14, mmenke wrote:
> > On 2014/07/31 15:38:17, pauljensen wrote:
> > > const?
> > 
> > Done.  Left on my own, I never mark functions that return non-const
references
> > to objects owned by a class const, because it seems like non-const-y
behavior
> to
> > me.
> The return type could be a const reference.

Done.  In fact it has to be.  Since the TestURLRequestContext actually owns the
delegate, which is passed in to ShouldRejectRequest as a non-const object, this
still seems rather non-const-correct to me, but I suppose that's more of an
issue with the context, than with this function.

[pauljensen, 2014-07-31 19:13:41]

lgtm.  You can't think of any cases where a URLRequestContext implementation
might be inherently tied to a NetworkDelegate implementation can you?  I can't. 
Probably wouldn't be a good idea.  I'm just mentioning this as it's a case this
change could break.

[mmenke, 2014-07-31 19:38:27]

On 2014/07/31 19:13:41, pauljensen wrote:
> lgtm.  You can't think of any cases where a URLRequestContext implementation
> might be inherently tied to a NetworkDelegate implementation can you?  I
can't. 
> Probably wouldn't be a good idea.  I'm just mentioning this as it's a case
this
> change could break.

I'm not aware of any.  No classes outside net/ that are injected into net/ get
access to the URLRequest* except the NetworkDelegate and URLRequest::Delegate,
so in terms of comparing points and the like, and making sure things are passed
through you, I don't think it's possible, as long as one entity sets both the
URLRequest::Delegate and the NetworkDelegate.  One could imagine lower level
dependencies, but I can't think of any.

This is a valid concern.  If we run into any issues may have to rethink how
we're going to get AppCache off of the old API.  Even if this turns out not to
work, we should still keep 99.9% of this CL, and just modify the URLRequest not
to take a network delegate as an argument.

[pauljensen, 2014-07-31 19:39:49]

Agreed.

[mmenke, 2014-07-31 19:40:05]

On 2014/07/31 19:38:27, mmenke wrote:
> On 2014/07/31 19:13:41, pauljensen wrote:
> > lgtm.  You can't think of any cases where a URLRequestContext implementation
> > might be inherently tied to a NetworkDelegate implementation can you?  I
> can't. 
> > Probably wouldn't be a good idea.  I'm just mentioning this as it's a case
> this
> > change could break.
> 
> I'm not aware of any.  No classes outside net/ that are injected into net/ get
> access to the URLRequest* except the NetworkDelegate and URLRequest::Delegate,
> so in terms of comparing points and the like, and making sure things are
passed
> through you, I don't think it's possible, as long as one entity sets both the
> URLRequest::Delegate and the NetworkDelegate.  One could imagine lower level
> dependencies, but I can't think of any.
> 
> This is a valid concern.  If we run into any issues may have to rethink how
> we're going to get AppCache off of the old API.  Even if this turns out not to
> work, we should still keep 99.9% of this CL, and just modify the URLRequest
not
> to take a network delegate as an argument.

Oh, injected URLRequestJobs / JobFactories / ProtocolHandlers get access to the
request, too.  Had forgotten about that.  That could theoretically be an issue. 
Think we're still safe, particularly if we stick to HTTP, but more of a concern
than I had been thinking.

[mmenke, 2014-08-05 21:53:36]

This is a largely mechanical change, replacing all "new URLRequest(blah, blah2,
blah3, context)" calls with "context->CreateRequest(blah, blah2, blah3, NULL);" 
Unfortunately, it affects code across a lot of boundaries, and some files needed
more significant changes than others (Removing URLRequest subclasses,
primarily).

jhawkins: chrome/browser/ (Except net/ and prerender/)
scottbyer: cloud_print/service/service_state.cc
bengr: components/data_reduction_proxy/
mkosiba: components/navigation_interception/
michaeln: content/browser/appcache/
yoz: extensions/browser/extension_protocols_unittest.cc
darin: mojo/services/network/url_loader_impl.cc

[Scott Byer, 2014-08-05 21:58:34]

LGTM for cloud print.

On 2014/08/05 21:53:36, mmenke wrote:
> This is a largely mechanical change, replacing all "new URLRequest(blah,
blah2,
> blah3, context)" calls with "context->CreateRequest(blah, blah2, blah3,
NULL);" 
> Unfortunately, it affects code across a lot of boundaries, and some files
needed
> more significant changes than others (Removing URLRequest subclasses,
> primarily).
> 
> jhawkins: chrome/browser/ (Except net/ and prerender/)
> scottbyer: cloud_print/service/service_state.cc
> bengr: components/data_reduction_proxy/
> mkosiba: components/navigation_interception/
> michaeln: content/browser/appcache/
> yoz: extensions/browser/extension_protocols_unittest.cc
> darin: mojo/services/network/url_loader_impl.cc

[bengr, 2014-08-05 22:08:02]

lgtm for data_reduction_proxy

[Yoyo Zhou, 2014-08-05 22:14:34]

extensions LGTM

[willchan no longer on Chromium, 2014-08-05 22:36:43]

Drive-by

https://codereview.chromium.org/407093011/diff/440001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/407093011/diff/440001/net/url_request/url_req...
net/url_request/url_request.h:270: // from the URLRequestContext.
Is it possible to be explicit all the time? Or is it too much of a PITA to fix
existing tests?

In general, I prefer being strict and explicit, rather than tolerant and falling
back. But I suspect you did this to reduce burden of updating tests, which is
valid to me. If so, that's probably worth documenting here.

[mmenke, 2014-08-05 22:42:34]

https://codereview.chromium.org/407093011/diff/440001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/407093011/diff/440001/net/url_request/url_req...
net/url_request/url_request.h:270: // from the URLRequestContext.
On 2014/08/05 22:36:43, willchan wrote:
> Is it possible to be explicit all the time? Or is it too much of a PITA to fix
> existing tests?
> 
> In general, I prefer being strict and explicit, rather than tolerant and
falling
> back. But I suspect you did this to reduce burden of updating tests, which is
> valid to me. If so, that's probably worth documenting here.

Being explicit on the cookie_store would be a huge pain (Unless I lifted that
logic into URLRequestContext::CreateRequest instead).

Being explicit on the network delegate is a 1-line change, but I thought it
would be better to be consistent with the cookie store.

Long term, as we lock down the URLRequestContext, we should hoist that logic up
to the to the URLRequestContext, and eventually get rid of it...  But I think we
should save that for when we're tearing apart the URLRequestContext /
URLRequestContextStorage, and figuring out how we want to handle that at the
same time.  The URLRequestContext may no longer keep a pointer to the cookie
store at that point, anyways.

Also, it will probably be another 3 years before we get to that point, and
someone else will probably be doing the work by then.  :)

[michaeln, 2014-08-05 22:56:16]

lgtm for content/browser/appcache/

qq: What does "allowing a URLRequestJob to transparently wrap a URLRequest"
mean? A description of the intended use case might help me understand what is
meant by that.

[mmenke, 2014-08-05 23:00:37]

On 2014/08/05 22:56:16, michaeln wrote:
> lgtm for content/browser/appcache/
> 
> qq: What does "allowing a URLRequestJob to transparently wrap a URLRequest"
> mean? A description of the intended use case might help me understand what is
> meant by that.

There's a description at the linked bug (Comment #5).  My general feeling is
that if you know what the difference between a URLRequest and a URLRequestJob
is, I think the idea follows fairly easily from that.  Including that level of
detail on every CL, as opposed to just the CL that actually implements that
behavior seems like overkill to me.  I could well be wrong on one or both of
those points.

[mmenke, 2014-08-05 23:03:16]

On 2014/08/05 23:00:37, mmenke wrote:
> On 2014/08/05 22:56:16, michaeln wrote:
> > lgtm for content/browser/appcache/
> > 
> > qq: What does "allowing a URLRequestJob to transparently wrap a URLRequest"
> > mean? A description of the intended use case might help me understand what
is
> > meant by that.
> 
> There's a description at the linked bug (Comment #5).  My general feeling is
> that if you know what the difference between a URLRequest and a URLRequestJob
> is, I think the idea follows fairly easily from that.  Including that level of
> detail on every CL, as opposed to just the CL that actually implements that
> behavior seems like overkill to me.  I could well be wrong on one or both of
> those points.

And I've fleshed out the description slightly.

[mkosiba (inactive), 2014-08-06 10:37:06]

components/navigation_interception/ LGTM

[mmenke, 2014-08-15 18:31:04]

jhawkins: Ping!  Please review chrome/browser/ (Except net/ and prerender/)
darin: Ping!  Please review mojo/services/network/url_loader_impl.cc

This is a largely mechanical change, replacing all "new URLRequest(blah, blah2,
blah3, context)" calls with "context->CreateRequest(blah, blah2, blah3, NULL);" 
Unfortunately, it affects code across a lot of boundaries, and some files needed
more significant changes than others (Removing URLRequest subclasses,
primarily).

[mmenke, 2014-08-19 15:02:43]

This is a largely mechanical change, replacing all "new URLRequest(blah, blah2,
blah3, context)" calls with "context->CreateRequest(blah, blah2, blah3, NULL);" 
Unfortunately, it affects code across a lot of boundaries, and some files needed
more significant changes than others (Removing URLRequest subclasses,
primarily).

In a followup pass, the URLRequest constructor will be made private.


[+hashimoto]  Please review
chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc
[+battre] Please review
chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
[+sky] Please review chrome/browser/custom_handlers/, mojo,
chrome/browser/policy/, and chrome/browser/search/ (The first two have no more
specific owners, the other two so small that they don't seem worth a separate
owner).

[sky, 2014-08-19 16:52:26]

Said files LGTM

[hashimoto, 2014-08-20 01:29:47]

chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc lgtm

[mmenke, 2014-08-21 14:05:17]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-08-21 14:06:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mmenke@chromium.org/407093011/520001

[commit-bot: I haz the power, 2014-08-21 16:32:30]

Committed patchset #16 (520001) as 291090

[milnerfamily007, 2014-09-30 15:16:23]

milnerfamily007@gmail.com changed reviewers:
+ milnerfamily007@gmail.com

[milnerfamily007, 2014-09-30 15:18:40]

milnerfamily007@gmail.com changed reviewers:
- battre@chromium.org, hashimoto@chromium.org, sky@chromium.org

[milnerfamily007, 2014-09-30 15:18:42]

lgtm

[lucyrodz07, 2016-12-31 17:40:51]

lucyrodz07@gmail.com changed reviewers:
+ lucyrodz07@gmail.com