Introduce a delegate to avoid hardcoding "chrome-extension" in net/.

Introduce a delegate to avoid hardcoding "chrome-extension" in net/.

Also, deal with a couple of TODOs in the throttling code, and remove
metrics we no longer need since we are no longer running field trials
for this code.

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

[Jói, 2012-06-04 10:46:00]

eroman: Friendly ping :)

[eroman, 2012-06-04 18:54:50]

thx for the reminder, had forgotten

On Mon, Jun 4, 2012 at 3:46 AM, <joi@chromium.org> wrote:

> eroman: Friendly ping :)
>
>
http://codereview.chromium.**org/10440119/<http://codereview.chromium.org/104...
>

[eroman, 2012-06-05 05:30:24]

@willchan: Do you envision NetworkDelegate as a dumping grounds for all of the
client --> network stack communication? If so, then this interface could be
folded into NetworkDelegate instead of introducing a more specific class.

@joi: looks mostly good after addressing the mutability of URLRequest*
parameter, and getting feedback from Will regarding NetworkDelegate.

http://codereview.chromium.org/10440119/diff/17001/net/url_request/url_reques...
File net/url_request/url_request_throttler_entry_interface.h (right):

http://codereview.chromium.org/10440119/diff/17001/net/url_request/url_reques...
net/url_request/url_request_throttler_entry_interface.h:35: virtual bool
ShouldRejectRequest(URLRequest* request,
Can this be |const URLRequest*| or |const URLRequest&| ?

Mutable requests concern me, since if the consumer has the ability to do things
like cancel them, the callsite which exposes this has to be prepared to check
for changes.

http://codereview.chromium.org/10440119/diff/17001/net/url_request/url_reques...
File net/url_request/url_request_throttler_manager.h (right):

http://codereview.chromium.org/10440119/diff/17001/net/url_request/url_reques...
net/url_request/url_request_throttler_manager.h:34: virtual bool
MayRejectRequest(URLRequest* request) = 0;
Same comment as earlier, please make |request| const. If delegates can cancel or
change the request, that complicates things. And I don't think it is necessary
here.

http://codereview.chromium.org/10440119/diff/17001/net/url_request/url_reques...
net/url_request/url_request_throttler_manager.h:52:
URLRequestThrottlerManagerDelegate* delegate);
Please describe ownership semantics (i.e. that |delegate| must remain alive). I
know this is following standard model, but always helps to document it anyway
for readers.

[eroman, 2012-06-05 05:30:52]

Actually add willchan this time ;)
Will, I have a small question for you in my earlier comment.

[willchan no longer on Chromium, 2012-06-05 05:43:16]

On 2012/06/05 05:30:52, eroman wrote:
> Actually add willchan this time ;)
> Will, I have a small question for you in my earlier comment.

Yep, use NetworkDelegate.

[Jói, 2012-06-05 16:39:13]

eroman: Please take another look.  I switched to using NetworkDelegate.

brettw: OWNERS approval for trivial changes in content/ and webkit/ that arise
from the change in net/. Since they are trivial, if all the ducks were to line
up in a row before I hear back, I would probably TBR.

Cheers,
Jói

http://codereview.chromium.org/10440119/diff/17001/net/url_request/url_reques...
File net/url_request/url_request_throttler_entry_interface.h (right):

http://codereview.chromium.org/10440119/diff/17001/net/url_request/url_reques...
net/url_request/url_request_throttler_entry_interface.h:35: virtual bool
ShouldRejectRequest(URLRequest* request,
On 2012/06/05 05:30:25, eroman wrote:
> Can this be |const URLRequest*| or |const URLRequest&| ?
> 
> Mutable requests concern me, since if the consumer has the ability to do
things
> like cancel them, the callsite which exposes this has to be prepared to check
> for changes.

Done.

http://codereview.chromium.org/10440119/diff/17001/net/url_request/url_reques...
File net/url_request/url_request_throttler_manager.h (right):

http://codereview.chromium.org/10440119/diff/17001/net/url_request/url_reques...
net/url_request/url_request_throttler_manager.h:34: virtual bool
MayRejectRequest(URLRequest* request) = 0;
On 2012/06/05 05:30:25, eroman wrote:
> Same comment as earlier, please make |request| const. If delegates can cancel
or
> change the request, that complicates things. And I don't think it is necessary
> here.

Done.

http://codereview.chromium.org/10440119/diff/17001/net/url_request/url_reques...
net/url_request/url_request_throttler_manager.h:52:
URLRequestThrottlerManagerDelegate* delegate);
On 2012/06/05 05:30:25, eroman wrote:
> Please describe ownership semantics (i.e. that |delegate| must remain alive).
I
> know this is following standard model, but always helps to document it anyway
> for readers.

This concern goes away with the switch to NetworkDelegate, since I now retrieve
that when needed via the URLRequest's URLRequestContext.

[brettw, 2012-06-05 19:37:34]

lgtm

[eroman, 2012-06-06 04:52:11]

http://codereview.chromium.org/10440119/diff/35002/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/10440119/diff/35002/net/base/network_delegate....
net/base/network_delegate.h:209: virtual bool OnCanRejectRequest(const
URLRequest& request) const = 0;
nit: Can the name have the name "throttle" in it somewhere? "Reject" request
seems a bit too generic, and makes me think of OnBeforeURLRequest(). Would
"OnCanThrottlerRejectRequest" or "OnCanThrottleRequest" work?

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
File net/url_request/url_request_throttler_entry.cc (right):

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
net/url_request/url_request_throttler_entry.cc:160: is_backoff_disabled_ = true;
Is this still used?

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
net/url_request/url_request_throttler_entry.cc:172:
request->context()->network_delegate()->CanRejectRequest(*request)) &&
Is it possible for network_delegate() to be NULL (haven't checked, just asking
to make sure).

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
File net/url_request/url_request_throttler_entry.h (right):

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
net/url_request/url_request_throttler_entry.h:97: int load_flags) const
OVERRIDE;
Is the load_flags parameter needed anymore? (since it is part of |request|).

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
File net/url_request/url_request_throttler_entry_interface.h (right):

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
net/url_request/url_request_throttler_entry_interface.h:39: int load_flags)
const = 0;
is the load_flags parameter needed?

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
File net/url_request/url_request_throttler_manager.h (right):

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
net/url_request/url_request_throttler_manager.h:20: #include
"net/url_request/url_request_context.h"
Is this header used?

[Jói, 2012-06-06 13:38:54]

Please take another look.

Cheers,
Jói

http://codereview.chromium.org/10440119/diff/35002/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/10440119/diff/35002/net/base/network_delegate....
net/base/network_delegate.h:209: virtual bool OnCanRejectRequest(const
URLRequest& request) const = 0;
On 2012/06/06 04:52:11, eroman wrote:
> nit: Can the name have the name "throttle" in it somewhere? "Reject" request
> seems a bit too generic, and makes me think of OnBeforeURLRequest(). Would
> "OnCanThrottlerRejectRequest" or "OnCanThrottleRequest" work?

Switched naming to CanThrottleRequest / OnCanThrottleRequest.

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
File net/url_request/url_request_throttler_entry.cc (right):

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
net/url_request/url_request_throttler_entry.cc:160: is_backoff_disabled_ = true;
On 2012/06/06 04:52:11, eroman wrote:
> Is this still used?

Yes, this is separate from the global flag; it's a way to disable throttling on
an entry, so that it only does the sliding window logic.  I have a separate TODO
to look at separating the sliding window logic from the throttling altogether,
but am not sure when I will get to that.

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
net/url_request/url_request_throttler_entry.cc:172:
request->context()->network_delegate()->CanRejectRequest(*request)) &&
On 2012/06/06 04:52:11, eroman wrote:
> Is it possible for network_delegate() to be NULL (haven't checked, just asking
> to make sure).

So say the trybots :)  Added that to the check.

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
File net/url_request/url_request_throttler_entry.h (right):

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
net/url_request/url_request_throttler_entry.h:97: int load_flags) const
OVERRIDE;
On 2012/06/06 04:52:11, eroman wrote:
> Is the load_flags parameter needed anymore? (since it is part of |request|).

You're right, removed.

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
File net/url_request/url_request_throttler_entry_interface.h (right):

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
net/url_request/url_request_throttler_entry_interface.h:39: int load_flags)
const = 0;
On 2012/06/06 04:52:11, eroman wrote:
> is the load_flags parameter needed?

Done.

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
File net/url_request/url_request_throttler_manager.h (right):

http://codereview.chromium.org/10440119/diff/35002/net/url_request/url_reques...
net/url_request/url_request_throttler_manager.h:20: #include
"net/url_request/url_request_context.h"
On 2012/06/06 04:52:11, eroman wrote:
> Is this header used?

Only in the .cc file, moved it there.

[eroman, 2012-06-07 02:20:10]

lgtm

http://codereview.chromium.org/10440119/diff/30011/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10440119/diff/30011/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:369: return
request.first_party_for_cookies().scheme() !=
nit: our style says to avoid writing "else" after return statements. (please
move this out of an else)

[Jói, 2012-06-07 11:00:15]

Thanks, committing.

http://codereview.chromium.org/10440119/diff/30011/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10440119/diff/30011/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:369: return
request.first_party_for_cookies().scheme() !=
On 2012/06/07 02:20:10, eroman wrote:
> nit: our style says to avoid writing "else" after return statements. (please
> move this out of an else)

Done.

[commit-bot: I haz the power, 2012-06-07 11:00:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10440119/28019

[commit-bot: I haz the power, 2012-06-07 13:21:17]

Change committed as 141000