Change many extension event routers to not be singletons and to be more profile-aware.

Change many extension event routers to not be singletons and to be more profile-aware.

Make these event routers owned by the profile via the ExtensionService, and only handle events for their profile:
- history (note: already doesn't work in incognito)
- bookmark
- cookies
- management
- web navigation

BUG=57186, 81745
TEST=covered by existing tests


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

[asargent_no_longer_on_chrome, 2011-06-23 22:18:38]

description nit: the second number on the BUG= line is wrong - I think you want
81745

http://codereview.chromium.org/7243012/diff/1/chrome/browser/chromeos/extensi...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/7243012/diff/1/chrome/browser/chromeos/extensi...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:78: if
(!profile_)
Should we have NOTREACHED() or CHECK(false) here instead of ignoring it?

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_bookmarks_module.cc (right):

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_bookmarks_module.cc:128:
model->AddObserver(this);
Is there a matching call to RemoveObserver somewhere? I expected to see it in
here somewhere.

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_history_api.cc (right):

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_history_api.cc:73: if (registrar_.IsEmpty())
{
since you removed the "this is safe to call multiple times" comment, should we
just CHECK(registrar_.IsEmpty()) at the beginning of this function?

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_management_api.cc (right):

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_management_api.cc:263: if
(registrar_.IsEmpty()) {
same question here - if we now expect to have one ExtensionManagementEventRouter
per Profile, should we CHECK that registrar_ is empty?

[Yoyo Zhou, 2011-06-24 17:21:38]

Fixed the BUG= line.

http://codereview.chromium.org/7243012/diff/1/chrome/browser/chromeos/extensi...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/7243012/diff/1/chrome/browser/chromeos/extensi...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:78: if
(!profile_)
On 2011/06/23 22:18:38, Antony Sargent wrote:
> Should we have NOTREACHED() or CHECK(false) here instead of ignoring it? 

Sure.

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_bookmarks_module.cc (right):

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_bookmarks_module.cc:128:
model->AddObserver(this);
On 2011/06/23 22:18:38, Antony Sargent wrote:
> Is there a matching call to RemoveObserver somewhere? I expected to see it in
> here somewhere.

There wasn't before... we should probably have one assuming the model hasn't
been destroyed by the time the router is destroyed. Can we count on that?

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_history_api.cc (right):

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_history_api.cc:73: if (registrar_.IsEmpty())
{
On 2011/06/23 22:18:38, Antony Sargent wrote:
> since you removed the "this is safe to call multiple times" comment, should we
> just CHECK(registrar_.IsEmpty()) at the beginning of this function?

Ok. (I don't know why we went to the effort of making things safe to call
multiple times when we never actually tried to call them multiple times.)

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_management_api.cc (right):

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_management_api.cc:263: if
(registrar_.IsEmpty()) {
On 2011/06/23 22:18:38, Antony Sargent wrote:
> same question here - if we now expect to have one
ExtensionManagementEventRouter
> per Profile, should we CHECK that registrar_ is empty? 

Done.

[asargent_no_longer_on_chrome, 2011-06-24 18:56:39]

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_bookmarks_module.cc (right):

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_bookmarks_module.cc:128:
model->AddObserver(this);
On 2011/06/24 17:21:38, Yoyo Zhou wrote:
> On 2011/06/23 22:18:38, Antony Sargent wrote:
> > Is there a matching call to RemoveObserver somewhere? I expected to see it
in
> > here somewhere.
> 
> There wasn't before... we should probably have one assuming the model hasn't
> been destroyed by the time the router is destroyed. Can we count on that?

Looks like BookmarkModelObserver's get their BookmarkModelBeingDeleted method
called when the model is being destructed, so we can just implement that and
drop our ptr to the model then, and skip calling RemoveObserver in our own
destructor here if the ptr is NULL.

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_history_api.cc (right):

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_history_api.cc:73: if (registrar_.IsEmpty())
{
> (I don't know why we went to the effort of making things safe to call
> multiple times when we never actually tried to call them multiple times.)

Me neither. =)

[Yoyo Zhou, 2011-06-24 22:10:33]

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_bookmarks_module.cc (right):

http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_bookmarks_module.cc:128:
model->AddObserver(this);
On 2011/06/24 18:56:39, Antony Sargent wrote:
> On 2011/06/24 17:21:38, Yoyo Zhou wrote:
> > On 2011/06/23 22:18:38, Antony Sargent wrote:
> > > Is there a matching call to RemoveObserver somewhere? I expected to see it
> in
> > > here somewhere.
> > 
> > There wasn't before... we should probably have one assuming the model hasn't
> > been destroyed by the time the router is destroyed. Can we count on that?
> 
> Looks like BookmarkModelObserver's get their BookmarkModelBeingDeleted method
> called when the model is being destructed, so we can just implement that and
> drop our ptr to the model then, and skip calling RemoveObserver in our own
> destructor here if the ptr is NULL. 
> 

We don't keep a pointer to the model anymore; I guess we would have to to use it
in our own destructor.

Both the ExtensionService and BookmarkModel are scoped_ptrs held by the Profile,
so they would get destroyed at approximately the same time.

[Yoyo Zhou, 2011-06-24 22:34:10]

On 2011/06/24 22:10:33, Yoyo Zhou wrote:
>
http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
> File chrome/browser/extensions/extension_bookmarks_module.cc (right):
> 
>
http://codereview.chromium.org/7243012/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_bookmarks_module.cc:128:
> model->AddObserver(this);
> On 2011/06/24 18:56:39, Antony Sargent wrote:
> > On 2011/06/24 17:21:38, Yoyo Zhou wrote:
> > > On 2011/06/23 22:18:38, Antony Sargent wrote:
> > > > Is there a matching call to RemoveObserver somewhere? I expected to see
it
> > in
> > > > here somewhere.
> > > 
> > > There wasn't before... we should probably have one assuming the model
hasn't
> > > been destroyed by the time the router is destroyed. Can we count on that?
> > 
> > Looks like BookmarkModelObserver's get their BookmarkModelBeingDeleted
method
> > called when the model is being destructed, so we can just implement that and
> > drop our ptr to the model then, and skip calling RemoveObserver in our own
> > destructor here if the ptr is NULL. 
> > 
> 
> We don't keep a pointer to the model anymore; I guess we would have to to use
it
> in our own destructor.
> 
> Both the ExtensionService and BookmarkModel are scoped_ptrs held by the
Profile,
> so they would get destroyed at approximately the same time.

As we discussed, the BookmarkModel might have a stale pointer to the router
unless we RemoveObserver. That is done.

[asargent_no_longer_on_chrome, 2011-06-24 22:38:02]

LGTM

[commit-bot: I haz the power, 2011-06-25 02:21:49]

Change committed as 90477

[zel, 2011-07-20 17:44:48]

LGTM