Adds the new URLRequest::OnBeforeNetworkStart hook to the

Adds the new URLRequest::OnBeforeNetworkStart hook to the
ResourceThrottle so that the ResourceScheduler can eventually throttle
closer to the start of network usage.

This is the second step in the design doc:
https://docs.google.com/document/d/1TSI3jwozVB_nueWJxzi8uKbgDfsGZRZqw8tvtD-P1Iw/edit?usp=sharing

The third step is to create the new ResourceScheduler.

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

[James Simonsen, 2014-01-08 23:33:12]

lgtm

This looks like a great plan. Thanks for tackling this.

You might want to CC someone still active on Chrome to review this too, just so
that we can spread the knowledge around a bit. Maybe Matt or Will?

[jkarlin, 2014-01-09 12:16:33]

asanka@chromium.org: Please review changes in download/

jochen@chromium.org: Please review changes in content/public/browser

willchan@chromium.org: CC'ing you to keep you updated on this work.

[jochen (gone - plz use gerrit), 2014-01-09 12:19:40]

lgtm

[asanka, 2014-01-09 16:06:11]

/download/ lgtm

[willchan no longer on Chromium, 2014-01-09 17:04:02]

/cc mmenke

I just had one nit about the new URLRequest::Delegate API that's implemented by
ResourceLoader. Since we expect to see more consumers of the network stack, I
think we should be careful about our API surface.

My comment is non-blocking and is just for your consideration. Everything else
LGTM.

https://codereview.chromium.org/129173002/diff/1/content/browser/loader/resou...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/129173002/diff/1/content/browser/loader/resou...
content/browser/loader/resource_loader.h:69: virtual void
OnBeforeNetworkStart(net::URLRequest* request,
Nit: why is this non-const? Is it desirable to allow mutations here?

[mmenke, 2014-01-09 17:52:21]

https://codereview.chromium.org/129173002/diff/1/content/browser/loader/resou...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/129173002/diff/1/content/browser/loader/resou...
content/browser/loader/resource_loader.h:69: virtual void
OnBeforeNetworkStart(net::URLRequest* request,
On 2014/01/09 17:04:03, willchan wrote:
> Nit: why is this non-const? Is it desirable to allow mutations here?

Resuming the request if you choose to pause it at this point is a non-const
operation.  It seems really weird to require you use some other pointer to the
request if you want to resume it, after having paused it at this point.

[willchan no longer on Chromium, 2014-01-09 18:00:35]

https://codereview.chromium.org/129173002/diff/1/content/browser/loader/resou...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/129173002/diff/1/content/browser/loader/resou...
content/browser/loader/resource_loader.h:69: virtual void
OnBeforeNetworkStart(net::URLRequest* request,
On 2014/01/09 17:52:22, mmenke wrote:
> On 2014/01/09 17:04:03, willchan wrote:
> > Nit: why is this non-const? Is it desirable to allow mutations here?
> 
> Resuming the request if you choose to pause it at this point is a non-const
> operation.  It seems really weird to require you use some other pointer to the
> request if you want to resume it, after having paused it at this point.

OK, that seems like a good reason. When I thought about it a bit more, the
URLRequest::Delegate is the owner after all, and so it should have non-const
access.

That said, it's not clear to me that all the ResourceLoader implementations
should have the same access. I guess if ResourceLoaders were internal to
content, I wouldn't worry. But CONTENT_EXPORT makes me think they're part of the
interface, which makes me nervous about extending the same API access to outside
consumers.

Not saying we should do it, but I think it's worth considering whether or not we
want to split out a separate implementation of URLRequest::Delegate that drives
the ResourceLoaders (I think this is RDH), and can give ResourceLoaders a const
URLRequest&, and provide Pause()/Start() APIs.

[commit-bot: I haz the power, 2014-01-09 18:34:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/129173002/1

[jkarlin, 2014-01-09 18:36:18]

On 2014/01/09 18:00:35, willchan wrote:
>
https://codereview.chromium.org/129173002/diff/1/content/browser/loader/resou...
> File content/browser/loader/resource_loader.h (right):
> 
>
https://codereview.chromium.org/129173002/diff/1/content/browser/loader/resou...
> content/browser/loader/resource_loader.h:69: virtual void
> OnBeforeNetworkStart(net::URLRequest* request,
> On 2014/01/09 17:52:22, mmenke wrote:
> > On 2014/01/09 17:04:03, willchan wrote:
> > > Nit: why is this non-const? Is it desirable to allow mutations here?
> > 
> > Resuming the request if you choose to pause it at this point is a non-const
> > operation.  It seems really weird to require you use some other pointer to
the
> > request if you want to resume it, after having paused it at this point.
> 
> OK, that seems like a good reason. When I thought about it a bit more, the
> URLRequest::Delegate is the owner after all, and so it should have non-const
> access.
> 
> That said, it's not clear to me that all the ResourceLoader implementations
> should have the same access. I guess if ResourceLoaders were internal to
> content, I wouldn't worry. But CONTENT_EXPORT makes me think they're part of
the
> interface, which makes me nervous about extending the same API access to
outside
> consumers.
> 
> Not saying we should do it, but I think it's worth considering whether or not
we
> want to split out a separate implementation of URLRequest::Delegate that
drives
> the ResourceLoaders (I think this is RDH), and can give ResourceLoaders a
const
> URLRequest&, and provide Pause()/Start() APIs.

Acknowledged.

[commit-bot: I haz the power, 2014-01-09 22:16:17]

Change committed as 243994

[darin (slow to review), 2014-07-25 20:26:49]

https://codereview.chromium.org/129173002/diff/1/content/public/browser/resou...
File content/public/browser/resource_throttle.h (right):

https://codereview.chromium.org/129173002/diff/1/content/public/browser/resou...
content/public/browser/resource_throttle.h:28: virtual void
OnBeforeNetworkStart(bool* defer) {}
Sorry, I only stumbled upon this now, but this method is pretty obviously
misnamed. It isn't consistent with the other methods on this interface. I might
suggest a name like WillStartUsingNetwork, and it seems like it should be listed
after WillStartRequest but before WillRedirectRequest, right so that the methods
are ordered in a sensible fashion.

[jkarlin, 2014-07-28 18:53:06]

On 2014/07/25 20:26:49, darin wrote:
>
https://codereview.chromium.org/129173002/diff/1/content/public/browser/resou...
> File content/public/browser/resource_throttle.h (right):
> 
>
https://codereview.chromium.org/129173002/diff/1/content/public/browser/resou...
> content/public/browser/resource_throttle.h:28: virtual void
> OnBeforeNetworkStart(bool* defer) {}
> Sorry, I only stumbled upon this now, but this method is pretty obviously
> misnamed. It isn't consistent with the other methods on this interface. I
might
> suggest a name like WillStartUsingNetwork, and it seems like it should be
listed
> after WillStartRequest but before WillRedirectRequest, right so that the
methods
> are ordered in a sensible fashion.

Thanks for the catch.  Out for review:
https://codereview.chromium.org/424753005/