Implementation of ExtensionEventRouterForwarder

Implementation of ExtensionEventRouterForwarder

Implementation of ExtensionEventRouterForwarder, a class that allows forwarding events from contexts without a profile (e.g. system URL request context) and from the IO thread to extensions.

BUG=73903
TEST=unit_tests --gtest_filter='ExtensionEventRouterForwarderTest.*'

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

[battre, 2011-02-22 17:34:43]

Please review. Thanks.

[jochen (gone - plz use gerrit), 2011-02-22 19:05:35]

+matt

matt, the idea is to get rid of the dependency of chrome network delegate on
chrome url request contexts.

Esp. for requests that are not associated to a profile (such as the proxy
fetcher requests) we can't rely on getting a profile pointer from somewhere

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder.cc (right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.cc:48: event_name,
event_args, (*i), event_url);
doesn't this fit on the previous line?

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder.h (right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.h:16: class GURL;
nit alphabet

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.h:58: protected:
you don't need protected twice. also, destructor should come before methods

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.h:64:
DISALLOW_COPY_AND_ASSIGN(ExtensionEventRouterForwarder);
empty line before this

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder_unittest.cc
(right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:80: void
createProfile(const std::string& dir) {
Can't you use testing profiles?

[Matt Perry, 2011-02-22 20:16:25]

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder.cc (right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.cc:19: void
ExtensionEventRouterForwarder::DispatchEventToRenderers(
This method is very similar to DispatchEventToExtension. How about you just call
that with an empty extension_id? DoDispatch can decide which to use based on
whether extension_id is empty.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.cc:38: LOG(WARNING)
<< "Did not send event " << event_name << "(" << event_args
This doesn't seem worth warning about to me.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.cc:58: event_name,
event_args, restrict_to_profile, event_url);
There's a subtle difference between dispatching from a profile's event router
and telling it to restrict to a profile. In other words, you can pass a NULL
profile to DispatchEventToXXX.

If you pass a profile, we will do filtering based on incognito/non-incognito.
For example, if restrict_to_profile is OTR, and the extension is not allowed to
see incognito events, we will not dispatch the event to that extension. However,
if you pass NULL for restrict_to_profile, we send the event regardless.

It depends on the event which way we want to go. For example, bookmark events
pass NULL, since they are not scoped to OTR.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder.h (right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.h:18: // This class
forwards events to ExtensionEventRouters.
Do you plan on having this class replace ExtensionIOEventRouter?

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder_unittest.cc
(right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:80: void
createProfile(const std::string& dir) {
C++ style: CreateProfile

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:80: void
createProfile(const std::string& dir) {
On 2011/02/22 19:05:35, jochen wrote:
> Can't you use testing profiles?

Agreed. It'd be a shame to have to hit the filesystem for this test.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:96:
Profile* getProfile(const std::string& dir) {
here too

[Matt Perry, 2011-02-22 20:16:25]

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder.cc (right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.cc:19: void
ExtensionEventRouterForwarder::DispatchEventToRenderers(
This method is very similar to DispatchEventToExtension. How about you just call
that with an empty extension_id? DoDispatch can decide which to use based on
whether extension_id is empty.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.cc:38: LOG(WARNING)
<< "Did not send event " << event_name << "(" << event_args
This doesn't seem worth warning about to me.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.cc:58: event_name,
event_args, restrict_to_profile, event_url);
There's a subtle difference between dispatching from a profile's event router
and telling it to restrict to a profile. In other words, you can pass a NULL
profile to DispatchEventToXXX.

If you pass a profile, we will do filtering based on incognito/non-incognito.
For example, if restrict_to_profile is OTR, and the extension is not allowed to
see incognito events, we will not dispatch the event to that extension. However,
if you pass NULL for restrict_to_profile, we send the event regardless.

It depends on the event which way we want to go. For example, bookmark events
pass NULL, since they are not scoped to OTR.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder.h (right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.h:18: // This class
forwards events to ExtensionEventRouters.
Do you plan on having this class replace ExtensionIOEventRouter?

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder_unittest.cc
(right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:80: void
createProfile(const std::string& dir) {
C++ style: CreateProfile

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:80: void
createProfile(const std::string& dir) {
On 2011/02/22 19:05:35, jochen wrote:
> Can't you use testing profiles?

Agreed. It'd be a shame to have to hit the filesystem for this test.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:96:
Profile* getProfile(const std::string& dir) {
here too

[jochen (gone - plz use gerrit), 2011-02-22 20:31:07]

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder.h (right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.h:18: // This class
forwards events to ExtensionEventRouters.
On 2011/02/22 20:16:25, Matt Perry wrote:
> Do you plan on having this class replace ExtensionIOEventRouter?
that's the idea

[Paweł Hajdan Jr., 2011-02-23 07:28:58]

Drive-by with minor testing-related comments. Please let me do another review
before committing.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder_unittest.cc
(right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:57:
ASSERT_TRUE(PathService::Get(base::DIR_TEMP, &test_dir_));
Please use ScopedTempDir instead.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:75: //
Clean up test directory
After using the scoped temp dir the explicit cleanup won't be needed.

[jochen (gone - plz use gerrit), 2011-02-23 08:35:36]

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder.cc (right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.cc:58: event_name,
event_args, restrict_to_profile, event_url);
On 2011/02/22 20:16:25, Matt Perry wrote:
> There's a subtle difference between dispatching from a profile's event router
> and telling it to restrict to a profile. In other words, you can pass a NULL
> profile to DispatchEventToXXX.
> 
> If you pass a profile, we will do filtering based on incognito/non-incognito.
> For example, if restrict_to_profile is OTR, and the extension is not allowed
to
> see incognito events, we will not dispatch the event to that extension.
However,
> if you pass NULL for restrict_to_profile, we send the event regardless.
> 
> It depends on the event which way we want to go. For example, bookmark events
> pass NULL, since they are not scoped to OTR.

I'd suggest to add separate methods for broadcasting events and for sending them
to a specific profile.

The latter class of methods should take an additional bool
"use_profile_to_restrict_events" (or similar/shorter).

[battre, 2011-02-23 14:12:17]

Thanks for your reviews. I have addressed the comments.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder.cc (right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.cc:19: void
ExtensionEventRouterForwarder::DispatchEventToRenderers(
On 2011/02/22 20:16:25, Matt Perry wrote:
> This method is very similar to DispatchEventToExtension. How about you just
call
> that with an empty extension_id? DoDispatch can decide which to use based on
> whether extension_id is empty.

Done.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.cc:38: LOG(WARNING)
<< "Did not send event " << event_name << "(" << event_args
On 2011/02/22 20:16:25, Matt Perry wrote:
> This doesn't seem worth warning about to me.

Done.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.cc:48: event_name,
event_args, (*i), event_url);
On 2011/02/22 19:05:35, jochen wrote:
> doesn't this fit on the previous line?

Done.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.cc:58: event_name,
event_args, restrict_to_profile, event_url);
On 2011/02/23 08:35:36, jochen wrote:
> On 2011/02/22 20:16:25, Matt Perry wrote:
> > There's a subtle difference between dispatching from a profile's event
router
> > and telling it to restrict to a profile. In other words, you can pass a NULL
> > profile to DispatchEventToXXX.
> > 
> > If you pass a profile, we will do filtering based on
incognito/non-incognito.
> > For example, if restrict_to_profile is OTR, and the extension is not allowed
> to
> > see incognito events, we will not dispatch the event to that extension.
> However,
> > if you pass NULL for restrict_to_profile, we send the event regardless.
> > 
> > It depends on the event which way we want to go. For example, bookmark
events
> > pass NULL, since they are not scoped to OTR.
> 
> I'd suggest to add separate methods for broadcasting events and for sending
them
> to a specific profile.
> 
> The latter class of methods should take an additional bool
> "use_profile_to_restrict_events" (or similar/shorter).

Done.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder.h (right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.h:16: class GURL;
On 2011/02/22 19:05:35, jochen wrote:
> nit alphabet

Done.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.h:58: protected:
On 2011/02/22 19:05:35, jochen wrote:
> you don't need protected twice. also, destructor should come before methods

Done.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.h:64:
DISALLOW_COPY_AND_ASSIGN(ExtensionEventRouterForwarder);
On 2011/02/22 19:05:35, jochen wrote:
> empty line before this

Done.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder_unittest.cc
(right):

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:57:
ASSERT_TRUE(PathService::Get(base::DIR_TEMP, &test_dir_));
On 2011/02/23 07:28:58, Paweł Hajdan Jr. wrote:
> Please use ScopedTempDir instead.

I have worked around requiring a full profile.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:75: //
Clean up test directory
On 2011/02/23 07:28:58, Paweł Hajdan Jr. wrote:
> After using the scoped temp dir the explicit cleanup won't be needed.

This code is gone but I have created a GoodFirstBug http://crbug.com/73854
because there are several places that set up temporary directories themselves.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:80: void
createProfile(const std::string& dir) {
On 2011/02/22 20:16:25, Matt Perry wrote:
> On 2011/02/22 19:05:35, jochen wrote:
> > Can't you use testing profiles?
> 
> Agreed. It'd be a shame to have to hit the filesystem for this test.

Done.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:80: void
createProfile(const std::string& dir) {
On 2011/02/22 20:16:25, Matt Perry wrote:
> C++ style: CreateProfile

gone.

http://codereview.chromium.org/6538093/diff/2004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:96:
Profile* getProfile(const std::string& dir) {
On 2011/02/22 20:16:25, Matt Perry wrote:
> here too

gone

[Matt Perry, 2011-02-23 19:22:12]

LG on my end.

http://codereview.chromium.org/6538093/diff/12002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_event_router_forwarder_unittest.cc
(right):

http://codereview.chromium.org/6538093/diff/12002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
optional: any way we can test what happens when mixing OTR profiles?

[battre, 2011-02-23 20:59:35]

Thanks.

http://codereview.chromium.org/6538093/diff/12002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_event_router_forwarder_unittest.cc
(right):

http://codereview.chromium.org/6538093/diff/12002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2011/02/23 19:22:12, Matt Perry wrote:
> optional: any way we can test what happens when mixing OTR profiles?

Done.

[jochen (gone - plz use gerrit), 2011-02-23 21:12:52]

lg

[commit-bot: I haz the power, 2011-02-23 22:01:12]

No LGTM from valid reviewers yet.

[jochen (gone - plz use gerrit), 2011-02-23 22:02:45]

On 2011/02/23 22:01:12, commit-bot wrote:
> No LGTM from valid reviewers yet.

LGTM

[commit-bot: I haz the power, 2011-02-23 22:45:05]

Change committed as 75807

[Paweł Hajdan Jr., 2011-02-25 17:57:35]

Code I commented in the drive-by LGTM with a comment.

http://codereview.chromium.org/6538093/diff/15002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_event_router_forwarder_unittest.cc
(right):

http://codereview.chromium.org/6538093/diff/15002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router_forwarder_unittest.cc:69:
ScopedTestingBrowserProcess browser_process_;
nit: I'd suggest inheriting from TestingBrowserProcess instead, so that
browser_process_ is the very first member variable (might help in tricky
situations).