(Owner approval for) Delay network requests on startup if any webRequest ...

Delay network requests on startup if any webRequest or webNavigation extensions are enabled.

Add a webRequest extension API permission, used to tell when an extension uses that API and therefore wants to delay startup. Use the "tabs" warning for it.

Also clean up the UserScriptListener, which never released requests individually and so doesn't need to track them individually either, and makes the RequestQueue handle bulk releases by its delegates instead.



BUG=99450
TEST=unit_tests.exe --gtest_filter=NetworkDelayListenerTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105659

[Pam (message me for reviews), 2011-10-07 14:42:51]

Still to come are unit tests (this CL) and hooking this into the notification
system for alerting users when there's a lengthy delay (another CL).

aa: please review, especially which notifications are being observed in
network_delay_listener.cc

battre, mpcomplete: fyi

Thanks,
- Pam

[Pam (message me for reviews), 2011-10-07 14:44:16]

Also the permissions. Right now I've added a webRequest permission (so I can
tell which extensions need to delay startup) and have it using the "tabs"
messaging.

- Pam

[battre, 2011-10-10 15:33:28]

drive-by and not in-depth review (I was curious :-))

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ne...
File chrome/browser/extensions/network_delay_listener.cc (right):

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ne...
chrome/browser/extensions/network_delay_listener.cc:123: NewRunnableMethod(
This is old-school ;-)

Check Albert J Wong's email "Removing NewRunnableMethod() and NewCallback()" to
chromium-dev.

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ne...
chrome/browser/extensions/network_delay_listener.cc:146: const Extension*
extension = eh ->extension();
nit: space before ->

[Aaron Boodman, 2011-10-10 22:59:29]

Nevermind what I suggested offline (about only blocking if there is someone
listening), I think it is more complexity than is probably warranted right now.

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2516:
SetDelaysNetworkRequests(extension,
This will definitely clear (as in remove from the file) the preference, right?
It would be a bummer to add a new "delaysNetworkRequests" key for every
extension.

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:198: void
SetDelaysNetworkRequests(const Extension* extension, bool delays);
Just call ExtensionPrefs directly for a few reasons:

1) This class is huge already.
2) Depending on ExtensionPrefs instead of ExtensionService from your listener
will make it easier to unit test (hint hint)

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ne...
File chrome/browser/extensions/network_delay_listener.cc (right):

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ne...
chrome/browser/extensions/network_delay_listener.cc:105:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
I think these should be CHECK. Being on the wrong thread is disastrous
eventually, and hard to debug. Better to crash obviously.

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ne...
File chrome/browser/extensions/network_delay_listener.h (right):

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ne...
chrome/browser/extensions/network_delay_listener.h:34: class
NetworkDelayListener
What happens if both this class and UserScriptListener try to delay a request.
Does it correctly wait for them both to sign off before executing the request?

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ne...
chrome/browser/extensions/network_delay_listener.h:41: // ResourceQueueDelegate:
You can make these private if you don't intend to unit test them or use them
other than through the interface.

http://codereview.chromium.org/8205001/diff/8001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/8205001/diff/8001/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.cc:314:
IDS_EXTENSION_PROMPT_WARNING_TABS,
What is the behavior with these two APIs if you have no host permissions. Do
they still give you URLs, or do they do nothing at all?

If the former, the warning you have here is appropriate. If the latter, there
should be no warning.

[Matt Perry, 2011-10-11 18:40:12]

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_listener.cc (left):

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_listener.cc:57:
delayed_request_ids_.push_front(request_id);
I'm confused by this change. Before, we only blocked requests that match a
pattern used by the set of loaded content scripts.

This change makes it so that we don't load any requests as long as there are
unloaded content scripts.

[Pam (message me for reviews), 2011-10-12 14:28:11]

All comments addressed, and unit tests added as promised. Please take another
look.

Thanks,
- Pam

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2516:
SetDelaysNetworkRequests(extension,
On 2011/10/10 22:59:29, Aaron Boodman wrote:
> This will definitely clear (as in remove from the file) the preference, right?

Yes, if false it removes the pref from the ExtensionPrefs dictionary entirely.

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ne...
File chrome/browser/extensions/network_delay_listener.h (right):

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/ne...
chrome/browser/extensions/network_delay_listener.h:34: class
NetworkDelayListener
On 2011/10/10 22:59:29, Aaron Boodman wrote:
> What happens if both this class and UserScriptListener try to delay a request.
> Does it correctly wait for them both to sign off before executing the request?

Yes, the ResourceQueue handles that when both are registered as delegates. I
added a unit test for it to resource_queue_unittests.cc.

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_listener.cc (left):

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_listener.cc:57:
delayed_request_ids_.push_front(request_id);
On 2011/10/11 18:40:12, Matt Perry wrote:
> I'm confused by this change. Before, we only blocked requests that match a
> pattern used by the set of loaded content scripts.
> 
> This change makes it so that we don't load any requests as long as there are
> unloaded content scripts.

We still only block requests that match a pattern (line 54 left or 53 right).

We used to, and still do, release all blocked requests at once (in
StartDelayedRequests) when we get NOTIFICATION_USER_SCRIPTS_UPDATED. That is,
all we ever did with delayed_request_ids_ is iterate over the whole thing. We
never released individual blocked requests, so there's no need for the added
complication of tracking them.

http://codereview.chromium.org/8205001/diff/8001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/8205001/diff/8001/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.cc:314:
IDS_EXTENSION_PROMPT_WARNING_TABS,
On 2011/10/10 22:59:29, Aaron Boodman wrote:
> What is the behavior with these two APIs if you have no host permissions. Do
> they still give you URLs, or do they do nothing at all?

> If the former, the warning you have here is appropriate. If the latter, there
> should be no warning.

They'll let you register a listener in any case, but if you don't have host
permission for the site that's generating the event, you won't get a
notification.

(At least, that's the design. For webRequest at least, it's not yet implemented,
but will be before it goes stable.)

Warnings removed.

http://codereview.chromium.org/8205001/diff/23007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/8205001/diff/23007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.h:198: // This should only be used by
unit tests.
I don't much like adding this, but the alternatives (either going to a ton of
work to build a real ExtensionHost in the unit test just to be able to send a
notification with it, or using a browser_test instead) were worse.

http://codereview.chromium.org/8205001/diff/23007/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/8205001/diff/23007/net/url_request/url_request...
net/url_request/url_request.h:41: class NetworkDelayListenerTest;
Sigh. I don't like doing this either, but the "temporary" hack has been in for
more than two months now, and it wasn't obvious to me how to convert the
UserScriptListenerTest on which this new one is based. It will be easier to
convert all these unit tests at once.

[Pam (message me for reviews), 2011-10-12 14:35:03]

http://codereview.chromium.org/8205001/diff/23007/chrome/browser/extensions/n...
File chrome/browser/extensions/network_delay_listener_unittest.cc (right):

http://codereview.chromium.org/8205001/diff/23007/chrome/browser/extensions/n...
chrome/browser/extensions/network_delay_listener_unittest.cc:58: explicit
TestExtensionHost(const Extension* extension,
Oops, I'll remove "explicit".

[Matt Perry, 2011-10-12 17:13:55]

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_listener.cc (left):

http://codereview.chromium.org/8205001/diff/8001/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_listener.cc:57:
delayed_request_ids_.push_front(request_id);
On 2011/10/12 14:28:12, Pam wrote:
> On 2011/10/11 18:40:12, Matt Perry wrote:
> > I'm confused by this change. Before, we only blocked requests that match a
> > pattern used by the set of loaded content scripts.
> > 
> > This change makes it so that we don't load any requests as long as there are
> > unloaded content scripts.
> 
> We still only block requests that match a pattern (line 54 left or 53 right).
> 
> We used to, and still do, release all blocked requests at once (in
> StartDelayedRequests) when we get NOTIFICATION_USER_SCRIPTS_UPDATED. That is,
> all we ever did with delayed_request_ids_ is iterate over the whole thing. We
> never released individual blocked requests, so there's no need for the added
> complication of tracking them.

Of course, good point! My mistake.

[Pam (message me for reviews), 2011-10-13 13:22:06]

Aaron,

The trybots look good, so please have another look. Thanks!
- Pam

[Aaron Boodman, 2011-10-13 18:53:03]

lgtm

[Pam (message me for reviews), 2011-10-14 07:50:44]

sky or jam: owner approval for content/browser/renderer_host/resource_queue*
please

jar: owner approval for net/url_request/url_request.h please

Thanks,
- Pam

[sky, 2011-10-14 15:01:05]

John is a better reviewer for all this than I am.

  -Scott

[jam, 2011-10-14 16:24:10]

content lgtm

[Pam (message me for reviews), 2011-10-14 18:31:04]

O Eric, Noble 'net/' Owner:

Could you take a look at the 2-line change in net/url_request/url_request.h
please?

Thanks,
- Pam

[willchan no longer on Chromium, 2011-10-14 19:23:08]

lgtm

[eroman, 2011-10-14 19:59:38]

lgtm for net/url_request

[commit-bot: I haz the power, 2011-10-14 20:55:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/8205001/39002

[commit-bot: I haz the power, 2011-10-14 22:16:39]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_clang, revision is 105573, job name
was 8205001-39002 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2011-10-14 22:20:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/8205001/39002

[commit-bot: I haz the power, 2011-10-15 01:07:05]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 105616, job name
was 8205001-39002 (previous was lost) (previous was lost) (retry) (previous was
lost).

[jar (doing other things), 2011-10-15 02:43:40]

As I mentioned... I had started to look at this earlier this morning... I only
had one comment so far... but it sounds like you got the review from others, so
I'll stop where I was.

http://codereview.chromium.org/8205001/diff/28020/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/8205001/diff/28020/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.h:199: ExtensionHost(const Extension*
extension, content::ViewType host_type);
Should you consider making this private, and then friending the tests?