Implement ResourceQueue, an object that makes it easy to delay starting

Implement ResourceQueue, an object that makes it easy to delay starting
requests in ResourceDispatcherHost until specified conditions are met.

Make UserScriptListener use ResourceQueue.

This is the first step toward waiting for the privacy blacklists to load.

TEST=Covered by unit_tests.
BUG=21541
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=34271

[Paweł Hajdan Jr., 2009-12-07 14:59:53]

darin, jam: Please review.
mpcomplete: FYI.

[darin (slow to review), 2009-12-07 22:30:31]

http://codereview.chromium.org/460108/diff/1/2
File chrome/browser/extensions/user_script_listener.cc (right):

http://codereview.chromium.org/460108/diff/1/2#newcode30
chrome/browser/extensions/user_script_listener.cc:30: const
ResourceDispatcherHostRequestInfo& request_info,
(it really seems like we should come up with shorter names for
BlahBlahRequestInfo and BlahBlahRequestID.)

http://codereview.chromium.org/460108/diff/1/4
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/460108/diff/1/4#newcode246
chrome/browser/renderer_host/resource_dispatcher_host.cc:246: :
resource_queue_(new ResourceQueue()),
does this need to be heap allocated?  why not just have ResourceQueue be an
ordinary member variable?

http://codereview.chromium.org/460108/diff/1/6
File chrome/browser/renderer_host/resource_dispatcher_host_request_info.cc
(right):

http://codereview.chromium.org/460108/diff/1/6#newcode53
chrome/browser/renderer_host/resource_dispatcher_host_request_info.cc:53: // In
tests, resource_handler_ may be NULL.
it is annoying when the test environment allows something to be null that would
never be null in chrome.  maybe we should fix the tests so that there is a dummy
ResourceHandler at least?

http://codereview.chromium.org/460108/diff/1/8
File chrome/browser/renderer_host/resource_queue.cc (right):

http://codereview.chromium.org/460108/diff/1/8#newcode20
chrome/browser/renderer_host/resource_queue.cc:20: // ResourceDispatcherHost by
not shutting it down in tests.
maybe you can have a global variable that suppresses DCHECKs here?  and then
only set that global variable to true in some specific unit tests where the RDH
is leaked?  that way you'll get the benefit of the DCHECKs in other tests.

http://codereview.chromium.org/460108/diff/1/9
File chrome/browser/renderer_host/resource_queue.h (right):

http://codereview.chromium.org/460108/diff/1/9#newcode30
chrome/browser/renderer_host/resource_queue.h:30: virtual void
OnResourceQueueShutdown() = 0;
nit: how about calling this WillShutdownResourceQueue instead?  that way the
name of the method indicates that this is happening just before ResourceQueue
shutdown.

http://codereview.chromium.org/460108/diff/1/9#newcode58
chrome/browser/renderer_host/resource_queue.h:58: void PushRequest(URLRequest*
request,
nit: how about AddRequest instead?

http://codereview.chromium.org/460108/diff/1/9#newcode63
chrome/browser/renderer_host/resource_queue.h:63: void
InvalidateGlobalRequestID(
nit: maybe call this RemoveRequest?

http://codereview.chromium.org/460108/diff/1/9#newcode68
chrome/browser/renderer_host/resource_queue.h:68: // to be delayed, the request
will be started.
please note the requirement that this must be called on the IO thread.

perhaps it would be better to do like render_widget_helper.h and just have
dividers that bracket the methods intended for UI thread use from the methods
intended for IO thread use.  that way you don't have to comment on each method.

[Paweł Hajdan Jr., 2009-12-08 10:18:28]

On 2009/12/07 22:30:31, darin wrote:
> http://codereview.chromium.org/460108/diff/1/4
> File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):
> 
> http://codereview.chromium.org/460108/diff/1/4#newcode246
> chrome/browser/renderer_host/resource_dispatcher_host.cc:246: :
> resource_queue_(new ResourceQueue()),
> does this need to be heap allocated?  why not just have ResourceQueue be an
> ordinary member variable?

I'd also prefer it to be one. The problem is: one can't forward-declare an inner
class (or struct). GlobalRequestID is an inner struct inside RDH. So in
resource_queue.h I have to #include resource_dispatcher_host.h (I need the
declaration of GlobalRequestID and I can't forward-declare it).

For ResourceQueue to be a member variable of RDH, I'd have to #include
resource_queue.h in resource_dispatcher_host.h. But in turn that means that we
have a circular #include and one or the other header (depending on the order or
#includes in cc file) misses a critical declaration.

I see three choices here:
a) leave it as is
b) make GlobalRequestID no longer an inner struct, but keep it in the same
header
c) as above, but extract it to a separate header (global_request_id.h?)

I'd prefer c) especially that it would allow ResourceQueue to break any
dependency on RDH, even the header #include. In turn, all its delegates would
also be quite well insulated from RDH. What do you think?

> http://codereview.chromium.org/460108/diff/1/6
> File chrome/browser/renderer_host/resource_dispatcher_host_request_info.cc
> (right):
> 
> http://codereview.chromium.org/460108/diff/1/6#newcode53
> chrome/browser/renderer_host/resource_dispatcher_host_request_info.cc:53: //
In
> tests, resource_handler_ may be NULL.
> it is annoying when the test environment allows something to be null that
would
> never be null in chrome.  maybe we should fix the tests so that there is a
dummy
> ResourceHandler at least?

Sure, done.

> http://codereview.chromium.org/460108/diff/1/8
> File chrome/browser/renderer_host/resource_queue.cc (right):
> 
> http://codereview.chromium.org/460108/diff/1/8#newcode20
> chrome/browser/renderer_host/resource_queue.cc:20: // ResourceDispatcherHost
by
> not shutting it down in tests.
> maybe you can have a global variable that suppresses DCHECKs here?  and then
> only set that global variable to true in some specific unit tests where the
RDH
> is leaked?  that way you'll get the benefit of the DCHECKs in other tests.

It seems very hacky to me. I experimented a little even before submitting the
first revision of the patch. Fixing the ResourceDispatcherHost's unit test is
quite easy. With UserScriptListener I was getting a lot of weird behavior after
trying to shut down RDH. But, after this change lands, I can simplify the
UserScriptListener test (I can just test it through ResourceQueueDelegate
interface now, no need to mess with RDH), and fix the RDH test.

I'd prefer not to make non-trivial changes to test code in this CL because with
that we can have more confidence that things don't get broken.

> http://codereview.chromium.org/460108/diff/1/9
> File chrome/browser/renderer_host/resource_queue.h (right):
> 
> http://codereview.chromium.org/460108/diff/1/9#newcode30
> chrome/browser/renderer_host/resource_queue.h:30: virtual void
> OnResourceQueueShutdown() = 0;
> nit: how about calling this WillShutdownResourceQueue instead?  that way the
> name of the method indicates that this is happening just before ResourceQueue
> shutdown.

Done.

> http://codereview.chromium.org/460108/diff/1/9#newcode58
> chrome/browser/renderer_host/resource_queue.h:58: void PushRequest(URLRequest*
> request,
> nit: how about AddRequest instead?

Done.

> http://codereview.chromium.org/460108/diff/1/9#newcode63
> chrome/browser/renderer_host/resource_queue.h:63: void
> InvalidateGlobalRequestID(
> nit: maybe call this RemoveRequest?

Done.

> http://codereview.chromium.org/460108/diff/1/9#newcode68
> chrome/browser/renderer_host/resource_queue.h:68: // to be delayed, the
request
> will be started.
> please note the requirement that this must be called on the IO thread.
> 
> perhaps it would be better to do like render_widget_helper.h and just have
> dividers that bracket the methods intended for UI thread use from the methods
> intended for IO thread use.  that way you don't have to comment on each
method.

Done.

[darin (slow to review), 2009-12-09 00:04:10]

On 2009/12/08 10:18:28, Paweł Hajdan Jr. wrote:
> On 2009/12/07 22:30:31, darin wrote:
> > http://codereview.chromium.org/460108/diff/1/4
> > File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):
> > 
> > http://codereview.chromium.org/460108/diff/1/4#newcode246
> > chrome/browser/renderer_host/resource_dispatcher_host.cc:246: :
> > resource_queue_(new ResourceQueue()),
> > does this need to be heap allocated?  why not just have ResourceQueue be an
> > ordinary member variable?
> 
> I'd also prefer it to be one. The problem is: one can't forward-declare an
inner
> class (or struct). GlobalRequestID is an inner struct inside RDH. So in
> resource_queue.h I have to #include resource_dispatcher_host.h (I need the
> declaration of GlobalRequestID and I can't forward-declare it).
> 
> For ResourceQueue to be a member variable of RDH, I'd have to #include
> resource_queue.h in resource_dispatcher_host.h. But in turn that means that we
> have a circular #include and one or the other header (depending on the order
or
> #includes in cc file) misses a critical declaration.
> 
> I see three choices here:
> a) leave it as is
> b) make GlobalRequestID no longer an inner struct, but keep it in the same
> header
> c) as above, but extract it to a separate header (global_request_id.h?)
>
> I'd prefer c) especially that it would allow ResourceQueue to break any
> dependency on RDH, even the header #include. In turn, all its delegates would
> also be quite well insulated from RDH. What do you think?

I also prefer option c!


> > http://codereview.chromium.org/460108/diff/1/8
> > File chrome/browser/renderer_host/resource_queue.cc (right):
> > 
> > http://codereview.chromium.org/460108/diff/1/8#newcode20
> > chrome/browser/renderer_host/resource_queue.cc:20: // ResourceDispatcherHost
> by
> > not shutting it down in tests.
> > maybe you can have a global variable that suppresses DCHECKs here?  and then
> > only set that global variable to true in some specific unit tests where the
> RDH
> > is leaked?  that way you'll get the benefit of the DCHECKs in other tests.
> 
> It seems very hacky to me. I experimented a little even before submitting the
> first revision of the patch. Fixing the ResourceDispatcherHost's unit test is
> quite easy. With UserScriptListener I was getting a lot of weird behavior
after
> trying to shut down RDH. But, after this change lands, I can simplify the
> UserScriptListener test (I can just test it through ResourceQueueDelegate
> interface now, no need to mess with RDH), and fix the RDH test.
> 
> I'd prefer not to make non-trivial changes to test code in this CL because
with
> that we can have more confidence that things don't get broken.

OK

[Paweł Hajdan Jr., 2009-12-09 13:43:32]

So... patch updated!

Please review the comments especially carefully. If some of them can be improved
to be more clear or less confusing, I'd be glad to fix that.

[Paweł Hajdan Jr., 2009-12-10 07:36:09]

ping!

[darin (slow to review), 2009-12-10 18:46:02]

looks good.  just minor nits:

http://codereview.chromium.org/460108/diff/2007/3014
File chrome/browser/extensions/user_script_listener.h (right):

http://codereview.chromium.org/460108/diff/2007/3014#newcode16
chrome/browser/extensions/user_script_listener.h:16: struct GlobalRequestID;
nit: list classes first followed by structs.  i.e., sort on the first letter of
the line instead of the type name.

http://codereview.chromium.org/460108/diff/2007/3017
File chrome/browser/renderer_host/cross_site_resource_handler.h (right):

http://codereview.chromium.org/460108/diff/2007/3017#newcode10
chrome/browser/renderer_host/cross_site_resource_handler.h:10: struct
GlobalRequestID;
same nit about sorting

http://codereview.chromium.org/460108/diff/2007/3019
File chrome/browser/renderer_host/download_resource_handler.h (right):

http://codereview.chromium.org/460108/diff/2007/3019#newcode14
chrome/browser/renderer_host/download_resource_handler.h:14: struct
DownloadBuffer;
ditto

http://codereview.chromium.org/460108/diff/2007/3021
File chrome/browser/renderer_host/global_request_id.h (right):

http://codereview.chromium.org/460108/diff/2007/3021#newcode8
chrome/browser/renderer_host/global_request_id.h:8: // Uniquely identifies a
URLRequest.
It'd be good to explain what a child_id and request_id are.

http://codereview.chromium.org/460108/diff/2007/3023
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/460108/diff/2007/3023#newcode34
chrome/browser/renderer_host/resource_dispatcher_host.h:34: struct
GlobalRequestID;
ditto

http://codereview.chromium.org/460108/diff/2007/3026
File chrome/browser/renderer_host/resource_queue.h (right):

http://codereview.chromium.org/460108/diff/2007/3026#newcode13
chrome/browser/renderer_host/resource_queue.h:13: struct GlobalRequestID;
ditto

[Paweł Hajdan Jr., 2009-12-10 19:03:35]

Patch updated, please take another look.

[darin (slow to review), 2009-12-10 19:08:48]

LGTM