Make URLRequestThrottlerManager a member of URLRequestContext.

Make URLRequestThrottlerManager a member of URLRequestContext.

This addresses a long-standing TODO and allows us to simplify the
logic in the class a bit as it now lives fully on the IO thread.  It
should also allow further cleanup in follow-up changes e.g. to stop
using scoped_refptr for the URLRequestThrottlerEntry instances.

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

[Jói, 2012-04-23 23:17:03]

This is a bit of a preview as I still have to fix url_fetcher_impl_unittest.cc
to not retrieve the URLRequestThrottlerManager via singleton, and I haven't run
it through trybots yet, but I wanted to send this out before I end my day to get
some feedback on whether the approach looks roughly right.

Feel free to just make hand-wavy comments now and do a more detailed review once
it's fully ready.

Cheers,
Jói

[Jói, 2012-04-24 15:24:36]

This is ready for review now.

Eric: Main reviewer.
Will: OWNERS approval for ProfileIOData stuff.

Cheers,
Jói

[eroman, 2012-04-24 19:05:56]

lgtm

http://codereview.chromium.org/10203002/diff/5007/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/10203002/diff/5007/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:121: #include
"net/url_request/url_request_throttler_manager.h"
Can this be removed as well?

http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:220:
context->set_throttler_manager(globals->throttler_manager.get());
I'm not sure that we would ever want the ProxyScriptFetcherContext to do
per-request throttling. I guess this is fine for generality if the throttling
manager checks each request.

http://codereview.chromium.org/10203002/diff/5007/chrome/service/cloud_print/...
File chrome/service/cloud_print/cloud_print_url_fetcher_unittest.cc (right):

http://codereview.chromium.org/10203002/diff/5007/chrome/service/cloud_print/...
chrome/service/cloud_print/cloud_print_url_fetcher_unittest.cc:143:
net::URLRequestThrottlerManager throttler_manager_;
I wonder if it wouldn't be better to declare this higher up so it outlives the
fetcher_ member.

Actually hmm, fetcher_ is refcounted making it hard to guarantee anything about
the lifetimes... how about making this be owned by TestCloudPrintURLFetcher?
I'll leave this up to the OWNER for this test.

http://codereview.chromium.org/10203002/diff/5007/content/common/net/url_fetc...
File content/common/net/url_fetcher_impl_unittest.cc (right):

http://codereview.chromium.org/10203002/diff/5007/content/common/net/url_fetc...
content/common/net/url_fetcher_impl_unittest.cc:42: explicit
ThrottlingTestURLRequestContextGetter(
no need for "explicit"

http://codereview.chromium.org/10203002/diff/5007/content/common/net/url_fetc...
content/common/net/url_fetcher_impl_unittest.cc:63:
net::URLRequestThrottlerManager* throttler_manager_;
Same issue -- kinda risky for a refcounted object to hold a pointer to a
non-refcounted guy.

http://codereview.chromium.org/10203002/diff/5007/content/common/net/url_fetc...
content/common/net/url_fetcher_impl_unittest.cc:281: explicit
CancelTestURLRequestContextGetter(
no need for "explicit" anymore

http://codereview.chromium.org/10203002/diff/5007/net/url_request/url_request...
File net/url_request/url_request_test_util.cc (right):

http://codereview.chromium.org/10203002/diff/5007/net/url_request/url_request...
net/url_request/url_request_test_util.cc:160: if (!throttler_manager())
nit: i strongly recommend putting multi-line statements within curlies.

[Jói, 2012-04-27 13:20:26]

Thanks Eric.  Patch set 4 responds to your feedback, and also fixes a few more
URLRequestContext implementations that I had missed.  Patch set 5 is just a pure
merge to LKGR on top of that.

Will:  I think all that's left is your approval for
chrome/browser/profiles/*io_data stuff.

Cheers,
Jói

http://codereview.chromium.org/10203002/diff/5007/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/10203002/diff/5007/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:121: #include
"net/url_request/url_request_throttler_manager.h"
On 2012/04/24 19:05:57, eroman wrote:
> Can this be removed as well?

Done.

http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:220:
context->set_throttler_manager(globals->throttler_manager.get());
On 2012/04/24 19:05:57, eroman wrote:
> I'm not sure that we would ever want the ProxyScriptFetcherContext to do
> per-request throttling. I guess this is fine for generality if the throttling
> manager checks each request.

Right, it checks each request for now, because extensions don't use a separate
URLRequestContext for their requests.

However, it's quite conceivable to provide a DoNothingURLRequestThrottlerManager
and use that in URLRequestContexts where appropriate.  If we want to do that,
I'd prefer to do it as a separate change, to leave the current change more like
a clean refactoring that keeps the existing behavior.

http://codereview.chromium.org/10203002/diff/5007/chrome/service/cloud_print/...
File chrome/service/cloud_print/cloud_print_url_fetcher_unittest.cc (right):

http://codereview.chromium.org/10203002/diff/5007/chrome/service/cloud_print/...
chrome/service/cloud_print/cloud_print_url_fetcher_unittest.cc:143:
net::URLRequestThrottlerManager throttler_manager_;
On 2012/04/24 19:05:57, eroman wrote:
> I wonder if it wouldn't be better to declare this higher up so it outlives the
> fetcher_ member.
> 
> Actually hmm, fetcher_ is refcounted making it hard to guarantee anything
about
> the lifetimes... how about making this be owned by TestCloudPrintURLFetcher?
> I'll leave this up to the OWNER for this test.

Made it a member of the TestCloudPrintURLFetcher.

http://codereview.chromium.org/10203002/diff/5007/content/common/net/url_fetc...
File content/common/net/url_fetcher_impl_unittest.cc (right):

http://codereview.chromium.org/10203002/diff/5007/content/common/net/url_fetc...
content/common/net/url_fetcher_impl_unittest.cc:42: explicit
ThrottlingTestURLRequestContextGetter(
On 2012/04/24 19:05:57, eroman wrote:
> no need for "explicit"

Done.

http://codereview.chromium.org/10203002/diff/5007/content/common/net/url_fetc...
content/common/net/url_fetcher_impl_unittest.cc:63:
net::URLRequestThrottlerManager* throttler_manager_;
On 2012/04/24 19:05:57, eroman wrote:
> Same issue -- kinda risky for a refcounted object to hold a pointer to a
> non-refcounted guy.

Fixed by having URLFetcherTest own a TestURLRequestContext instead, and passing
it to the ThrottlingTestURLRequestContextGetter.

http://codereview.chromium.org/10203002/diff/5007/content/common/net/url_fetc...
content/common/net/url_fetcher_impl_unittest.cc:281: explicit
CancelTestURLRequestContextGetter(
On 2012/04/24 19:05:57, eroman wrote:
> no need for "explicit" anymore

Done.

http://codereview.chromium.org/10203002/diff/5007/net/url_request/url_request...
File net/url_request/url_request_test_util.cc (right):

http://codereview.chromium.org/10203002/diff/5007/net/url_request/url_request...
net/url_request/url_request_test_util.cc:160: if (!throttler_manager())
On 2012/04/24 19:05:57, eroman wrote:
> nit: i strongly recommend putting multi-line statements within curlies.

Done, and I totally agree, that's my usual style.

[Jói, 2012-04-27 13:25:00]

Adding John for content/OWNERS approval.

CCing OWNERS for trivial changes under chrome/browser/sync/, remoting/ and
webkit/; I don't plan to wait for LGTM for those bits.

Cheers,
Jói

[jam, 2012-04-27 15:04:58]

lgtm for content/shell (eroman has owners in content/common/net)

is a throttle manager always required, or is a NULL one handled? if so, it would
be nice to not set it in content_shell since it doesn't need it

[willchan no longer on Chromium, 2012-04-27 15:19:58]

On 2012/04/27 15:04:58, John Abd-El-Malek wrote:
> lgtm for content/shell (eroman has owners in content/common/net)
> 
> is a throttle manager always required, or is a NULL one handled? if so, it
would
> be nice to not set it in content_shell since it doesn't need it

Oops, my bad, I never finished this review. It was open in a tab :( I'll finish
it when I get into the office this morning.

[willchan no longer on Chromium, 2012-04-27 17:39:25]

The profile/ code is correct. But I'm not a big fan of updating all the other
URLRequestContexts.

http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:220:
context->set_throttler_manager(globals->throttler_manager.get());
On 2012/04/27 13:20:26, Jói wrote:
> On 2012/04/24 19:05:57, eroman wrote:
> > I'm not sure that we would ever want the ProxyScriptFetcherContext to do
> > per-request throttling. I guess this is fine for generality if the
throttling
> > manager checks each request.
> 
> Right, it checks each request for now, because extensions don't use a separate
> URLRequestContext for their requests.
> 
> However, it's quite conceivable to provide a
DoNothingURLRequestThrottlerManager
> and use that in URLRequestContexts where appropriate.  If we want to do that,
> I'd prefer to do it as a separate change, to leave the current change more
like
> a clean refactoring that keeps the existing behavior.

Why don't we just check NULL in URLRequestHttpJob? Why are we updating all this
code when really the only ones that should be invoking this code at all are
extension requests (main url request context)?

http://codereview.chromium.org/10203002/diff/28018/chrome/browser/policy/devi...
File chrome/browser/policy/device_management_service.cc (right):

http://codereview.chromium.org/10203002/diff/28018/chrome/browser/policy/devi...
chrome/browser/policy/device_management_service.cc:208:
set_throttler_manager(base_context->throttler_manager());
why?

[Jói, 2012-04-27 18:32:06]

I was thinking it was an awful lot of work adding this to
URLRequestContext, because of the many implementations.  Switching to
a check for NULL in the right place sounds good, I will work on that.

Cheers,
Jói


On Fri, Apr 27, 2012 at 5:39 PM,  <willchan@chromium.org> wrote:
> The profile/ code is correct. But I'm not a big fan of updating all the
> other
> URLRequestContexts.
>
>
>
> http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc
> File chrome/browser/io_thread.cc (right):
>
>
http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc...
> chrome/browser/io_thread.cc:220:
> context->set_throttler_manager(globals->throttler_manager.get());
> On 2012/04/27 13:20:26, Jói wrote:
>>
>> On 2012/04/24 19:05:57, eroman wrote:
>> > I'm not sure that we would ever want the ProxyScriptFetcherContext
>
> to do
>>
>> > per-request throttling. I guess this is fine for generality if the
>
> throttling
>>
>> > manager checks each request.
>
>
>> Right, it checks each request for now, because extensions don't use a
>
> separate
>>
>> URLRequestContext for their requests.
>
>
>> However, it's quite conceivable to provide a
>
> DoNothingURLRequestThrottlerManager
>>
>> and use that in URLRequestContexts where appropriate.  If we want to
>
> do that,
>>
>> I'd prefer to do it as a separate change, to leave the current change
>
> more like
>>
>> a clean refactoring that keeps the existing behavior.
>
>
> Why don't we just check NULL in URLRequestHttpJob? Why are we updating
> all this code when really the only ones that should be invoking this
> code at all are extension requests (main url request context)?
>
>
http://codereview.chromium.org/10203002/diff/28018/chrome/browser/policy/devi...
> File chrome/browser/policy/device_management_service.cc (right):
>
>
http://codereview.chromium.org/10203002/diff/28018/chrome/browser/policy/devi...
> chrome/browser/policy/device_management_service.cc:208:
> set_throttler_manager(base_context->throttler_manager());
> why?
>
> http://codereview.chromium.org/10203002/

[Jói, 2012-04-27 22:05:24]

Just realized something:  URLRequestThrottler* contains two things.
One is the exponential back-off behavior which is now used only for
requests originated by extensions.  The other is the "sliding window"
behavior that is used by URLFetcherCore to determine how much to delay
subsequent requests.  If I make all but the main URLRequestContext use
a NULL URLRequestThrottler, we lose this behavior.

Are we OK with losing this behavior for many URLRequestContexts
(always using a delay of 0 for
URLFetcherCore::StartURLRequestWhenAppropriate)?  If not I can suggest
a few different approaches:

a) Leave the change as is for now, then figure something out, which
might be one of the below;

b) Make the net::URLRequestContext base class instantiate and own the
URLRequestThrottlerManager; subclasses could still override but
wouldn't have to know about it if they don't need to override it (and
in practice right now, nobody would override it);

c) Provide an interface for URLRequestThrottlerManager and two
implementations of it, one of which only does the sliding window
thing, and have URLRequestContext use that as the default, similar to
(b).  The main Chrome URLRequestContext would override this to use the
throttler manager that also knows about exponential back-off.

The reason I didn't think about (b) in my initial change is that it
seems counter to the established pattern for URLRequestContext, but
maybe that is OK?

Cheers,
Jói


On Fri, Apr 27, 2012 at 6:31 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> I was thinking it was an awful lot of work adding this to
> URLRequestContext, because of the many implementations.  Switching to
> a check for NULL in the right place sounds good, I will work on that.
>
> Cheers,
> Jói
>
>
> On Fri, Apr 27, 2012 at 5:39 PM,  <willchan@chromium.org> wrote:
>> The profile/ code is correct. But I'm not a big fan of updating all the
>> other
>> URLRequestContexts.
>>
>>
>>
>>
http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc
>> File chrome/browser/io_thread.cc (right):
>>
>>
http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc...
>> chrome/browser/io_thread.cc:220:
>> context->set_throttler_manager(globals->throttler_manager.get());
>> On 2012/04/27 13:20:26, Jói wrote:
>>>
>>> On 2012/04/24 19:05:57, eroman wrote:
>>> > I'm not sure that we would ever want the ProxyScriptFetcherContext
>>
>> to do
>>>
>>> > per-request throttling. I guess this is fine for generality if the
>>
>> throttling
>>>
>>> > manager checks each request.
>>
>>
>>> Right, it checks each request for now, because extensions don't use a
>>
>> separate
>>>
>>> URLRequestContext for their requests.
>>
>>
>>> However, it's quite conceivable to provide a
>>
>> DoNothingURLRequestThrottlerManager
>>>
>>> and use that in URLRequestContexts where appropriate.  If we want to
>>
>> do that,
>>>
>>> I'd prefer to do it as a separate change, to leave the current change
>>
>> more like
>>>
>>> a clean refactoring that keeps the existing behavior.
>>
>>
>> Why don't we just check NULL in URLRequestHttpJob? Why are we updating
>> all this code when really the only ones that should be invoking this
>> code at all are extension requests (main url request context)?
>>
>>
http://codereview.chromium.org/10203002/diff/28018/chrome/browser/policy/devi...
>> File chrome/browser/policy/device_management_service.cc (right):
>>
>>
http://codereview.chromium.org/10203002/diff/28018/chrome/browser/policy/devi...
>> chrome/browser/policy/device_management_service.cc:208:
>> set_throttler_manager(base_context->throttler_manager());
>> why?
>>
>> http://codereview.chromium.org/10203002/

[willchan no longer on Chromium, 2012-04-27 22:17:53]

I think I'm fine losing the behavior. The overwhelming majority of our
URLFetchers use the main request context. All the rest use the system
request context in IOThread. If we wanted, we could set the
URLRequestThrottlerManager in the system request context.

On Fri, Apr 27, 2012 at 3:04 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Just realized something:  URLRequestThrottler* contains two things.
> One is the exponential back-off behavior which is now used only for
> requests originated by extensions.  The other is the "sliding window"
> behavior that is used by URLFetcherCore to determine how much to delay
> subsequent requests.  If I make all but the main URLRequestContext use
> a NULL URLRequestThrottler, we lose this behavior.
>
> Are we OK with losing this behavior for many URLRequestContexts
> (always using a delay of 0 for
> URLFetcherCore::StartURLRequestWhenAppropriate)?  If not I can suggest
> a few different approaches:
>
> a) Leave the change as is for now, then figure something out, which
> might be one of the below;
>
> b) Make the net::URLRequestContext base class instantiate and own the
> URLRequestThrottlerManager; subclasses could still override but
> wouldn't have to know about it if they don't need to override it (and
> in practice right now, nobody would override it);
>
> c) Provide an interface for URLRequestThrottlerManager and two
> implementations of it, one of which only does the sliding window
> thing, and have URLRequestContext use that as the default, similar to
> (b).  The main Chrome URLRequestContext would override this to use the
> throttler manager that also knows about exponential back-off.
>
> The reason I didn't think about (b) in my initial change is that it
> seems counter to the established pattern for URLRequestContext, but
> maybe that is OK?
>
> Cheers,
> Jói
>
>
> On Fri, Apr 27, 2012 at 6:31 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> > I was thinking it was an awful lot of work adding this to
> > URLRequestContext, because of the many implementations.  Switching to
> > a check for NULL in the right place sounds good, I will work on that.
> >
> > Cheers,
> > Jói
> >
> >
> > On Fri, Apr 27, 2012 at 5:39 PM,  <willchan@chromium.org> wrote:
> >> The profile/ code is correct. But I'm not a big fan of updating all the
> >> other
> >> URLRequestContexts.
> >>
> >>
> >>
> >>
> http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc
> >> File chrome/browser/io_thread.cc (right):
> >>
> >>
>
http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc...
> >> chrome/browser/io_thread.cc:220:
> >> context->set_throttler_manager(globals->throttler_manager.get());
> >> On 2012/04/27 13:20:26, Jói wrote:
> >>>
> >>> On 2012/04/24 19:05:57, eroman wrote:
> >>> > I'm not sure that we would ever want the ProxyScriptFetcherContext
> >>
> >> to do
> >>>
> >>> > per-request throttling. I guess this is fine for generality if the
> >>
> >> throttling
> >>>
> >>> > manager checks each request.
> >>
> >>
> >>> Right, it checks each request for now, because extensions don't use a
> >>
> >> separate
> >>>
> >>> URLRequestContext for their requests.
> >>
> >>
> >>> However, it's quite conceivable to provide a
> >>
> >> DoNothingURLRequestThrottlerManager
> >>>
> >>> and use that in URLRequestContexts where appropriate.  If we want to
> >>
> >> do that,
> >>>
> >>> I'd prefer to do it as a separate change, to leave the current change
> >>
> >> more like
> >>>
> >>> a clean refactoring that keeps the existing behavior.
> >>
> >>
> >> Why don't we just check NULL in URLRequestHttpJob? Why are we updating
> >> all this code when really the only ones that should be invoking this
> >> code at all are extension requests (main url request context)?
> >>
> >>
>
http://codereview.chromium.org/10203002/diff/28018/chrome/browser/policy/devi...
> >> File chrome/browser/policy/device_management_service.cc (right):
> >>
> >>
>
http://codereview.chromium.org/10203002/diff/28018/chrome/browser/policy/devi...
> >> chrome/browser/policy/device_management_service.cc:208:
> >> set_throttler_manager(base_context->throttler_manager());
> >> why?
> >>
> >> http://codereview.chromium.org/10203002/
>

[Jói, 2012-04-27 22:31:35]

OK cool.  Just asking because that behavior has been around since way
before the URLRequestThrottlerManager was created, it just migrated
there at the time.  Personally I think it's probably fine to remove it
for contexts other than main and system.

Cheers,
Jói


On Fri, Apr 27, 2012 at 10:17 PM, William Chan (陈智昌)
<willchan@chromium.org> wrote:
> I think I'm fine losing the behavior. The overwhelming majority of our
> URLFetchers use the main request context. All the rest use the system
> request context in IOThread. If we wanted, we could set the
> URLRequestThrottlerManager in the system request context.
>
>
> On Fri, Apr 27, 2012 at 3:04 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> Just realized something:  URLRequestThrottler* contains two things.
>> One is the exponential back-off behavior which is now used only for
>> requests originated by extensions.  The other is the "sliding window"
>> behavior that is used by URLFetcherCore to determine how much to delay
>> subsequent requests.  If I make all but the main URLRequestContext use
>> a NULL URLRequestThrottler, we lose this behavior.
>>
>> Are we OK with losing this behavior for many URLRequestContexts
>> (always using a delay of 0 for
>> URLFetcherCore::StartURLRequestWhenAppropriate)?  If not I can suggest
>> a few different approaches:
>>
>> a) Leave the change as is for now, then figure something out, which
>> might be one of the below;
>>
>> b) Make the net::URLRequestContext base class instantiate and own the
>> URLRequestThrottlerManager; subclasses could still override but
>> wouldn't have to know about it if they don't need to override it (and
>> in practice right now, nobody would override it);
>>
>> c) Provide an interface for URLRequestThrottlerManager and two
>> implementations of it, one of which only does the sliding window
>> thing, and have URLRequestContext use that as the default, similar to
>> (b).  The main Chrome URLRequestContext would override this to use the
>> throttler manager that also knows about exponential back-off.
>>
>> The reason I didn't think about (b) in my initial change is that it
>> seems counter to the established pattern for URLRequestContext, but
>> maybe that is OK?
>>
>> Cheers,
>> Jói
>>
>>
>> On Fri, Apr 27, 2012 at 6:31 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> > I was thinking it was an awful lot of work adding this to
>> > URLRequestContext, because of the many implementations.  Switching to
>> > a check for NULL in the right place sounds good, I will work on that.
>> >
>> > Cheers,
>> > Jói
>> >
>> >
>> > On Fri, Apr 27, 2012 at 5:39 PM,  <willchan@chromium.org> wrote:
>> >> The profile/ code is correct. But I'm not a big fan of updating all the
>> >> other
>> >> URLRequestContexts.
>> >>
>> >>
>> >>
>> >>
>> >>
http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc
>> >> File chrome/browser/io_thread.cc (right):
>> >>
>> >>
>> >>
http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc...
>> >> chrome/browser/io_thread.cc:220:
>> >> context->set_throttler_manager(globals->throttler_manager.get());
>> >> On 2012/04/27 13:20:26, Jói wrote:
>> >>>
>> >>> On 2012/04/24 19:05:57, eroman wrote:
>> >>> > I'm not sure that we would ever want the ProxyScriptFetcherContext
>> >>
>> >> to do
>> >>>
>> >>> > per-request throttling. I guess this is fine for generality if the
>> >>
>> >> throttling
>> >>>
>> >>> > manager checks each request.
>> >>
>> >>
>> >>> Right, it checks each request for now, because extensions don't use a
>> >>
>> >> separate
>> >>>
>> >>> URLRequestContext for their requests.
>> >>
>> >>
>> >>> However, it's quite conceivable to provide a
>> >>
>> >> DoNothingURLRequestThrottlerManager
>> >>>
>> >>> and use that in URLRequestContexts where appropriate.  If we want to
>> >>
>> >> do that,
>> >>>
>> >>> I'd prefer to do it as a separate change, to leave the current change
>> >>
>> >> more like
>> >>>
>> >>> a clean refactoring that keeps the existing behavior.
>> >>
>> >>
>> >> Why don't we just check NULL in URLRequestHttpJob? Why are we updating
>> >> all this code when really the only ones that should be invoking this
>> >> code at all are extension requests (main url request context)?
>> >>
>> >>
>> >>
http://codereview.chromium.org/10203002/diff/28018/chrome/browser/policy/devi...
>> >> File chrome/browser/policy/device_management_service.cc (right):
>> >>
>> >>
>> >>
http://codereview.chromium.org/10203002/diff/28018/chrome/browser/policy/devi...
>> >> chrome/browser/policy/device_management_service.cc:208:
>> >> set_throttler_manager(base_context->throttler_manager());
>> >> why?
>> >>
>> >> http://codereview.chromium.org/10203002/
>
>

[willchan no longer on Chromium, 2012-04-27 22:32:59]

Great, me too. Cheers.

On Fri, Apr 27, 2012 at 3:30 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> OK cool.  Just asking because that behavior has been around since way
> before the URLRequestThrottlerManager was created, it just migrated
> there at the time.  Personally I think it's probably fine to remove it
> for contexts other than main and system.
>
> Cheers,
> Jói
>
>
> On Fri, Apr 27, 2012 at 10:17 PM, William Chan (陈智昌)
> <willchan@chromium.org> wrote:
> > I think I'm fine losing the behavior. The overwhelming majority of our
> > URLFetchers use the main request context. All the rest use the system
> > request context in IOThread. If we wanted, we could set the
> > URLRequestThrottlerManager in the system request context.
> >
> >
> > On Fri, Apr 27, 2012 at 3:04 PM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >>
> >> Just realized something:  URLRequestThrottler* contains two things.
> >> One is the exponential back-off behavior which is now used only for
> >> requests originated by extensions.  The other is the "sliding window"
> >> behavior that is used by URLFetcherCore to determine how much to delay
> >> subsequent requests.  If I make all but the main URLRequestContext use
> >> a NULL URLRequestThrottler, we lose this behavior.
> >>
> >> Are we OK with losing this behavior for many URLRequestContexts
> >> (always using a delay of 0 for
> >> URLFetcherCore::StartURLRequestWhenAppropriate)?  If not I can suggest
> >> a few different approaches:
> >>
> >> a) Leave the change as is for now, then figure something out, which
> >> might be one of the below;
> >>
> >> b) Make the net::URLRequestContext base class instantiate and own the
> >> URLRequestThrottlerManager; subclasses could still override but
> >> wouldn't have to know about it if they don't need to override it (and
> >> in practice right now, nobody would override it);
> >>
> >> c) Provide an interface for URLRequestThrottlerManager and two
> >> implementations of it, one of which only does the sliding window
> >> thing, and have URLRequestContext use that as the default, similar to
> >> (b).  The main Chrome URLRequestContext would override this to use the
> >> throttler manager that also knows about exponential back-off.
> >>
> >> The reason I didn't think about (b) in my initial change is that it
> >> seems counter to the established pattern for URLRequestContext, but
> >> maybe that is OK?
> >>
> >> Cheers,
> >> Jói
> >>
> >>
> >> On Fri, Apr 27, 2012 at 6:31 PM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >> > I was thinking it was an awful lot of work adding this to
> >> > URLRequestContext, because of the many implementations.  Switching to
> >> > a check for NULL in the right place sounds good, I will work on that.
> >> >
> >> > Cheers,
> >> > Jói
> >> >
> >> >
> >> > On Fri, Apr 27, 2012 at 5:39 PM,  <willchan@chromium.org> wrote:
> >> >> The profile/ code is correct. But I'm not a big fan of updating all
> the
> >> >> other
> >> >> URLRequestContexts.
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
> http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc
> >> >> File chrome/browser/io_thread.cc (right):
> >> >>
> >> >>
> >> >>
>
http://codereview.chromium.org/10203002/diff/5007/chrome/browser/io_thread.cc...
> >> >> chrome/browser/io_thread.cc:220:
> >> >> context->set_throttler_manager(globals->throttler_manager.get());
> >> >> On 2012/04/27 13:20:26, Jói wrote:
> >> >>>
> >> >>> On 2012/04/24 19:05:57, eroman wrote:
> >> >>> > I'm not sure that we would ever want the ProxyScriptFetcherContext
> >> >>
> >> >> to do
> >> >>>
> >> >>> > per-request throttling. I guess this is fine for generality if the
> >> >>
> >> >> throttling
> >> >>>
> >> >>> > manager checks each request.
> >> >>
> >> >>
> >> >>> Right, it checks each request for now, because extensions don't use
> a
> >> >>
> >> >> separate
> >> >>>
> >> >>> URLRequestContext for their requests.
> >> >>
> >> >>
> >> >>> However, it's quite conceivable to provide a
> >> >>
> >> >> DoNothingURLRequestThrottlerManager
> >> >>>
> >> >>> and use that in URLRequestContexts where appropriate.  If we want to
> >> >>
> >> >> do that,
> >> >>>
> >> >>> I'd prefer to do it as a separate change, to leave the current
> change
> >> >>
> >> >> more like
> >> >>>
> >> >>> a clean refactoring that keeps the existing behavior.
> >> >>
> >> >>
> >> >> Why don't we just check NULL in URLRequestHttpJob? Why are we
> updating
> >> >> all this code when really the only ones that should be invoking this
> >> >> code at all are extension requests (main url request context)?
> >> >>
> >> >>
> >> >>
>
http://codereview.chromium.org/10203002/diff/28018/chrome/browser/policy/devi...
> >> >> File chrome/browser/policy/device_management_service.cc (right):
> >> >>
> >> >>
> >> >>
>
http://codereview.chromium.org/10203002/diff/28018/chrome/browser/policy/devi...
> >> >> chrome/browser/policy/device_management_service.cc:208:
> >> >> set_throttler_manager(base_context->throttler_manager());
> >> >> why?
> >> >>
> >> >> http://codereview.chromium.org/10203002/
> >
> >
>

[darin (slow to review), 2012-04-30 06:16:46]

LGTM for webkit/

[Jói, 2012-05-02 16:15:00]

Will, please take another look.  I've made URLRequestThrottlerManager optional
on the URLRequestContext, which makes this change much smaller in terms of the
number of files it touches.

Cheers,
Jói

[willchan no longer on Chromium, 2012-05-02 16:40:07]

lgtm

http://codereview.chromium.org/10203002/diff/47004/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10203002/diff/47004/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:222:
context->set_throttler_manager(globals->throttler_manager.get());
Just doublechecking you want this one. This is for the PAC script fetches. If
you want it, then OK.

http://codereview.chromium.org/10203002/diff/47004/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/10203002/diff/47004/chrome/browser/io_thread.h...
chrome/browser/io_thread.h:111: scoped_ptr<net::URLRequestThrottlerManager>
throttler_manager;
Move this above |system_request_context|. Since this is referenced by the
URLRequestContext, it should outlive it.

http://codereview.chromium.org/10203002/diff/47004/content/common/net/url_fet...
File content/common/net/url_fetcher_core.cc (right):

http://codereview.chromium.org/10203002/diff/47004/content/common/net/url_fet...
content/common/net/url_fetcher_core.cc:668: // URLRequestThrottler code does not
necessarily back off on the
URLRequestThrottlerManager?

[Jói, 2012-05-02 16:45:28]

Thanks, good catch on the system_url_request_context.

Cheers,
Jói

http://codereview.chromium.org/10203002/diff/47004/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10203002/diff/47004/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:222:
context->set_throttler_manager(globals->throttler_manager.get());
On 2012/05/02 16:40:07, willchan wrote:
> Just doublechecking you want this one. This is for the PAC script fetches. If
> you want it, then OK.

On further reflection I don't think we need it there.  Removed.

http://codereview.chromium.org/10203002/diff/47004/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/10203002/diff/47004/chrome/browser/io_thread.h...
chrome/browser/io_thread.h:111: scoped_ptr<net::URLRequestThrottlerManager>
throttler_manager;
On 2012/05/02 16:40:07, willchan wrote:
> Move this above |system_request_context|. Since this is referenced by the
> URLRequestContext, it should outlive it.

Done.

http://codereview.chromium.org/10203002/diff/47004/content/common/net/url_fet...
File content/common/net/url_fetcher_core.cc (right):

http://codereview.chromium.org/10203002/diff/47004/content/common/net/url_fet...
content/common/net/url_fetcher_core.cc:668: // URLRequestThrottler code does not
necessarily back off on the
On 2012/05/02 16:40:07, willchan wrote:
> URLRequestThrottlerManager?

Changed to "URLRequestThrottlerManager and related code"

[commit-bot: I haz the power, 2012-05-02 17:42:05]

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

[commit-bot: I haz the power, 2012-05-02 19:20:18]

Change committed as 134963