Change event routers from singletons to being owned by the ExtensionService.

Change event routers from singletons to being owned by the ExtensionService.

ExtensionService is in turn owned by the Profile.

Also stop pretending that each event router observes more than one profile. (To support multi-profile, each profile would have its own ExtensionService and routers.)

BUG=81745
TEST=covered by existing tests

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

[asargent_no_longer_on_chrome, 2011-05-20 21:03:34]

The change looks great in general. The only think I think needs fixing is that
it's probably a bad idea to push all the initialization stuff into the
constructors, for the reasons outlined here:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_...

[Yoyo Zhou, 2011-05-20 21:55:25]

On 2011/05/20 21:03:34, Antony Sargent wrote:
> The change looks great in general. The only think I think needs fixing is that
> it's probably a bad idea to push all the initialization stuff into the
> constructors, for the reasons outlined here:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_...

Ok, I guess I should undo some of that. Trybots found me another place that
needs to change, browser/ui/views/accessibility_event_router_views.cc. It has
some unusual profile handling, so I'm not sure what to do with it.

[Yoyo Zhou, 2011-05-24 17:11:15]

I addressed your comments, and also made some necessary changes for
accessibility in views (+dmazzoni for that).

[asargent_no_longer_on_chrome, 2011-05-24 18:18:00]

A few nits. Also you should change the "TEST=" line to be something like
"TEST=Should be covered by existing tests". 

(The idea of the TEST= line is to have a human readable description for our QA
team to look at for each change so they know where to focus their attention when
looking at the set of diffs included in a new build.)

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:62: }
nit: I think this code block should be moved into an Init method as well.

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_bookmarks_module.h (right):

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_bookmarks_module.h:28: virtual
~ExtensionBookmarkEventRouter() {}
nit: probably shouldn't inline the constructor/destructor here. See "Stop
inlining constructors and destructors" section of:

http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_cookies_api.h (right):

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_cookies_api.h:34: virtual
~ExtensionCookiesEventRouter() {}
same nit here about inlining

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_history_api.h (right):

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_history_api.h:25: virtual
~ExtensionHistoryEventRouter() {}
same inlining nit here

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_management_api.h (right):

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_management_api.h:53: virtual
~ExtensionManagementEventRouter() {}
same inlining nit here

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webnavigation_api.h (right):

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webnavigation_api.h:135: virtual
~ExtensionWebNavigationEventRouter() {}
same inlining nit here

[dmazzoni, 2011-05-24 18:19:52]

LGTM for *accessibility*. Thanks.

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/ui/views/acce...
File chrome/browser/ui/views/accessibility_event_router_views.cc (right):

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/ui/views/acce...
chrome/browser/ui/views/accessibility_event_router_views.cc:50: // We check if
accessibility is enabled in DispatchAccessibilityNotification,
I don't think this comment is needed, it's only helpful when understanding this
changelist. When looking at the code fresh, it's clearer without any comment
here.

[Yoyo Zhou, 2011-05-24 19:17:29]

On 2011/05/24 18:19:52, Dominic Mazzoni wrote:
> LGTM for *accessibility*. Thanks.

FYI, I found another spot (wizard_accessibility_helper.cc) that I had to change.
It does look like we only ever SetAccessibilityEnabled on the default profile's
ExtensionAccessibilityEventRouter, but we read it from whichever profile is
associated with the current view. This might become buggy once we have
multi-profiles.

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/ui/views/acce...
> File chrome/browser/ui/views/accessibility_event_router_views.cc (right):
> 
>
http://codereview.chromium.org/6961027/diff/4001/chrome/browser/ui/views/acce...
> chrome/browser/ui/views/accessibility_event_router_views.cc:50: // We check if
> accessibility is enabled in DispatchAccessibilityNotification,
> I don't think this comment is needed, it's only helpful when understanding
this
> changelist. When looking at the code fresh, it's clearer without any comment
> here.

Ok, deleted comment.

[Yoyo Zhou, 2011-05-24 19:17:55]

I updated the TEST= description.

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:62: }
On 2011/05/24 18:18:00, Antony Sargent wrote:
> nit: I think this code block should be moved into an Init method as well. 

Done.

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_bookmarks_module.h (right):

http://codereview.chromium.org/6961027/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_bookmarks_module.h:28: virtual
~ExtensionBookmarkEventRouter() {}
On 2011/05/24 18:18:00, Antony Sargent wrote:
> nit: probably shouldn't inline the constructor/destructor here. See "Stop
> inlining constructors and destructors" section of:
> 
> http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts

Done here and elsewhere.

[dmazzoni, 2011-05-24 19:27:20]

On Tue, May 24, 2011 at 12:17 PM, <yoz@chromium.org> wrote:

> On 2011/05/24 18:19:52, Dominic Mazzoni wrote:
>
>> LGTM for *accessibility*. Thanks.
>>
>
> FYI, I found another spot (wizard_accessibility_helper.cc) that I had to
> change.
> It does look like we only ever SetAccessibilityEnabled on the default
> profile's
> ExtensionAccessibilityEventRouter, but we read it from whichever profile is
> associated with the current view. This might become buggy once we have
> multi-profiles.


SetAccessibilityEnabled can be called by an extension for desktop Chrome, so
I think it's okay...but I'll check the ChromeOS case. I don't know what
multi-profile will look like in ChromeOS anyway.

- Dominic

[asargent_no_longer_on_chrome, 2011-05-24 19:53:31]

LGTM

http://codereview.chromium.org/6961027/diff/7002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/6961027/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:202: Profile* profile)  :
profile_(profile) {
did you mean to include this extra space be here?

[Yoyo Zhou, 2011-05-25 00:48:46]

Okay, I've painted myself into a corner.

In accessibility_event_router_views_unittest.cc, we try to
SetAccesibilityEnabled(true) on the accessibility event router. Previously it
was a singleton. Now it has to be owned by the TestingProfile in this test via
its ExtensionService.

But now we can't use a real ExtensionService (via CreateExtensionService),
because that introduces dependencies on all the little requirements for every
router that gets initialized in InitEventRouters. And we can't use a
TestExtensionService mock because the profile only supports ExtensionService
(not ExtensionServiceInterface).

To me, it seems like the easiest dependency to break is from the
AccessibilityEventRouterViews to the profile's
ExtensionAccessibilityEventRouter; there could be some kind of accessibility
state provider (implemented by either the event router or a mock).

Is this a good idea? Do examples of something like this exist elsewhere? Do you
have better suggestions?

http://codereview.chromium.org/6961027/diff/7002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/6961027/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:202: Profile* profile)  :
profile_(profile) {
On 2011/05/24 19:53:31, Antony Sargent wrote:
> did you mean to include this extra space be here?

Nope, thanks.

[dmazzoni, 2011-05-25 01:13:48]

If the only problem is the unit test and the code otherwise looks fine, I'd
suggest doing something even simpler. How about:

bool AccessibilityEventRouterViews::IsAccessibilityEnabled(Profile* profile)
{
  if (accessibility_enabled_override_for_testing_) {
    return true;
  }

  DCHECK(profile);
  return profile->GetExtensionService()->
      accessibility_event_router()->IsAccessibilityEnabled();
}

// private method, callable from AccessibilityEventRouterViewsTest because
it's a friend class.
void AccessibilityEventRouterViews::SetAccessibilityEnabledForTesting() {
  accessibility_enabled_override_for_testing_ = true;
}

- Dominic

On Tue, May 24, 2011 at 5:48 PM, <yoz@chromium.org> wrote:

> Okay, I've painted myself into a corner.
>
> In accessibility_event_router_views_unittest.cc, we try to
> SetAccesibilityEnabled(true) on the accessibility event router. Previously
> it
> was a singleton. Now it has to be owned by the TestingProfile in this test
> via
> its ExtensionService.
>
> But now we can't use a real ExtensionService (via CreateExtensionService),
> because that introduces dependencies on all the little requirements for
> every
> router that gets initialized in InitEventRouters. And we can't use a
> TestExtensionService mock because the profile only supports
> ExtensionService
> (not ExtensionServiceInterface).
>
> To me, it seems like the easiest dependency to break is from the
> AccessibilityEventRouterViews to the profile's
> ExtensionAccessibilityEventRouter; there could be some kind of
> accessibility
> state provider (implemented by either the event router or a mock).
>
> Is this a good idea? Do examples of something like this exist elsewhere? Do
> you
> have better suggestions?
>
>
>
>
>
http://codereview.chromium.org/6961027/diff/7002/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_preference_api.cc (right):
>
>
>
http://codereview.chromium.org/6961027/diff/7002/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_preference_api.cc:202: Profile*
> profile)  : profile_(profile) {
> On 2011/05/24 19:53:31, Antony Sargent wrote:
>
>> did you mean to include this extra space be here?
>>
>
> Nope, thanks.
>
>
> http://codereview.chromium.org/6961027/
>

[asargent_no_longer_on_chrome, 2011-05-25 04:52:14]

Dominic's suggestion seems fine to me. If that seems troublesome, another
possibility you could look into is using a TestingProfile
(chrome/test/testing_profile.h) instead of a real Profile, and have that own a
TestExtensionService you create.

[Yoyo Zhou, 2011-05-25 06:16:12]

On 2011/05/25 04:52:14, Antony Sargent wrote:
> Dominic's suggestion seems fine to me. If that seems troublesome, another
> possibility you could look into is using a TestingProfile
> (chrome/test/testing_profile.h) instead of a real Profile, and have that own a
> TestExtensionService you create.

The test already uses a TestingProfile, which unfortunately doesn't take a
TestExtensionService.

[Yoyo Zhou, 2011-05-25 19:33:05]

I've implemented Dominic's suggestion. It's not quite that clean since the
override value has to be both false and true.

Another look?

[dmazzoni, 2011-05-25 20:17:39]

LGTM.

Yeah, slightly uglier with two booleans but still preferable to adding lots of
infrastructure to load the full extensions routing system when it's not really
needed for this test.

Thanks.

[commit-bot: I haz the power, 2011-05-26 00:09:52]

Change committed as 86750