Lazy initialization for PushMessagingEventRouter

Lazy initialization for PushMessagingEventRouter

This CL also solves the circular dependency between ProfileSyncService 
and ExtensionService (crbug.com/153827) by destroying
PushMessagingEventRouter before ProfileSyncService.

Patch from Joe Thomas <mhx348@motorola.com>;.

BUG=159265, 153827
TBR=ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172876

[Joe Thomas, 2012-12-10 03:29:02]

+ben for gyp
+akalin for sync/
+mirandac profiles/

[Miranda Callahan, 2012-12-10 14:07:37]

On 2012/12/10 03:29:02, Joe Thomas wrote:
> +ben for gyp
> +akalin for sync/
> +mirandac profiles/

profiles/* LGTM.

[Yoyo Zhou, 2012-12-10 20:06:01]

Add 153827 to the BUG= line. (I think this fixes it!)

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:315: //
TODO(akalin): Move this block to Shutdown() once
I believe you can do this and get rid of OnProfileSyncServiceShutdown
altogether.

akalin: do you agree?

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/push_messaging/push_messaging_api_factory.cc
(right):

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/push_messaging/push_messaging_api_factory.cc:28:
DependsOn(extensions::ExtensionSystemFactory::GetInstance());
According to crbug.com/153827, this should also depend on ProfileSyncService.
That would cause this to be destroyed before ProfileSyncService, which should
fix that bug.

[akalin, 2012-12-10 20:37:11]

Refer to bug 153827 in CL description also

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:315: //
TODO(akalin): Move this block to Shutdown() once
On 2012/12/10 20:06:01, Yoyo Zhou wrote:
> I believe you can do this and get rid of OnProfileSyncServiceShutdown
> altogether.
> 
> akalin: do you agree?

Yeah, per your other comment, I agree.

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:110: public
extensions::EventRouter::Observer {
indent with 'public' on the previous line

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/push_messaging/push_messaging_api_factory.cc
(right):

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/push_messaging/push_messaging_api_factory.cc:28:
DependsOn(extensions::ExtensionSystemFactory::GetInstance());
On 2012/12/10 20:06:01, Yoyo Zhou wrote:
> According to crbug.com/153827, this should also depend on ProfileSyncService.
> That would cause this to be destroyed before ProfileSyncService, which should
> fix that bug.

Cool, yeah, that should fix that bug.

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/push_messaging/push_messaging_api_factory.h
(left):

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/push_messaging/push_messaging_api_factory.h:2: //
Use of this source code is governed by a BSD-style license that can be
can you tweak the similarity settings to treat this as a new file?

https://codereview.chromium.org/11496004/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/11496004/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:28: #include
"chrome/browser/extensions/api/push_messaging/push_messaging_api.h"
remove this too

https://codereview.chromium.org/11496004/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:519: // TODO(akalin): Remove this
once http://crbug.com/153827 is fixed.
remove this too

[Joe Thomas, 2012-12-10 22:43:58]

Thanks for the review. I could not solve the similarity issue while uploading
the new files. Tried with git cl upload --similarity=100 but that did not solve
the problem. Could you please check whether the command I used is correct.?

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:315: //
TODO(akalin): Move this block to Shutdown() once
On 2012/12/10 20:06:01, Yoyo Zhou wrote:
> I believe you can do this and get rid of OnProfileSyncServiceShutdown
> altogether.
> 
> akalin: do you agree?

Done.

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:110: public
extensions::EventRouter::Observer {
On 2012/12/10 20:37:12, akalin wrote:
> indent with 'public' on the previous line

Done.

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/push_messaging/push_messaging_api_factory.cc
(right):

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/push_messaging/push_messaging_api_factory.cc:28:
DependsOn(extensions::ExtensionSystemFactory::GetInstance());
On 2012/12/10 20:06:01, Yoyo Zhou wrote:
> According to crbug.com/153827, this should also depend on ProfileSyncService.
> That would cause this to be destroyed before ProfileSyncService, which should
> fix that bug.

Done.

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/push_messaging/push_messaging_api_factory.h
(left):

https://codereview.chromium.org/11496004/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/push_messaging/push_messaging_api_factory.h:2: //
Use of this source code is governed by a BSD-style license that can be
On 2012/12/10 20:37:12, akalin wrote:
> can you tweak the similarity settings to treat this as a new file?

I tried with "git cl upload --similarity=100" but it does not solve the issue.

https://codereview.chromium.org/11496004/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/11496004/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:28: #include
"chrome/browser/extensions/api/push_messaging/push_messaging_api.h"
On 2012/12/10 20:37:12, akalin wrote:
> remove this too

Done.

https://codereview.chromium.org/11496004/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:519: // TODO(akalin): Remove this
once http://crbug.com/153827 is fixed.
On 2012/12/10 20:37:12, akalin wrote:
> remove this too

Done.

[Yoyo Zhou, 2012-12-10 22:52:49]

LGTM

https://codereview.chromium.org/11496004/diff/3002/chrome/browser/extensions/...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/11496004/diff/3002/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:79: void
PushMessagingEventRouter::Shutdown() {
Do we even need this anymore? I think we can just destroy the event router in
PushMessagingAPI::Shutdown.

https://codereview.chromium.org/11496004/diff/3002/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:80: // We
need an explicit Shutdown() due to the dependencies among the various
This comment should be removed, in any case.

[Joe Thomas, 2012-12-10 23:24:08]

https://codereview.chromium.org/11496004/diff/3002/chrome/browser/extensions/...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/11496004/diff/3002/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:79: void
PushMessagingEventRouter::Shutdown() {
On 2012/12/10 22:52:49, Yoyo Zhou wrote:
> Do we even need this anymore? I think we can just destroy the event router in
> PushMessagingAPI::Shutdown.

I think it's not required. Removed

https://codereview.chromium.org/11496004/diff/3002/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:80: // We
need an explicit Shutdown() due to the dependencies among the various
On 2012/12/10 22:52:49, Yoyo Zhou wrote:
> This comment should be removed, in any case.

Done.

[Yoyo Zhou, 2012-12-10 23:39:13]

Cool, LGTM.

[akalin, 2012-12-10 23:44:13]

one more comment

https://codereview.chromium.org/11496004/diff/3002/chrome/browser/extensions/...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

https://codereview.chromium.org/11496004/diff/3002/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:118:
PushMessagingEventRouter* push_messaging_event_router();
looks like this is used only for testing.  Can you rename to
GetPushMessagingEventRouterForTest()?

Does the test need the lazy-init behavior?  If not, can you make it just a plain
accessor and put the lazy-init in OnListenerAdded?  If it does, maybe you can
make the test explicitly initialize the router (in SetUp) or something?

[Ben Goodger (Google), 2012-12-11 01:18:26]

gyp lgtm

[Joe Thomas, 2012-12-11 21:57:03]

Please review the latest patch.

Thanks,
Joe

https://codereview.chromium.org/11496004/diff/3002/chrome/browser/extensions/...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

https://codereview.chromium.org/11496004/diff/3002/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:118:
PushMessagingEventRouter* push_messaging_event_router();
On 2012/12/10 23:44:13, akalin wrote:
> looks like this is used only for testing.  Can you rename to
> GetPushMessagingEventRouterForTest()?
> 
> Does the test need the lazy-init behavior?  If not, can you make it just a
plain
> accessor and put the lazy-init in OnListenerAdded?  If it does, maybe you can
> make the test explicitly initialize the router (in SetUp) or something?

Done. Initializing the event router on SetUpOnMainThread().

[Yoyo Zhou, 2012-12-11 22:07:27]

LGTM

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:310: void
PushMessagingAPI::InitializePushMessagingEventRouterForTest() {
It seems to me this isn't necessary for the tests that navigate to
event_dispatch.html (because that page adds a listener). For the others, it
would probably be sufficient to initialize the event router in
GetPushMessagingEventRouterForTest below.

If that passes the tests, I think it would be cleaner. Otherwise, this is fine
too.

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:123: // For
testing purpose.
nit: purposes

[akalin, 2012-12-11 22:21:53]

i think one more round to iron out the comments below

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:300:
ExtensionSystem::Get(profile_)->event_router()->UnregisterObserver(this);
Perhaps add a comment saying that it's okay to unregister an observer more than
once.

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:306:
push_messaging_event_router_.reset(new PushMessagingEventRouter(profile_));
perhaps DCHECK that push_messaging_event_router_ is NULL before resetting (since
we expect this to be called at most once, right?)

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:311:
push_messaging_event_router_.reset(new PushMessagingEventRouter(profile_));
can probably just call OnListenerAdded() directly here.

[akalin, 2012-12-11 22:23:40]

oops, missed the important comment

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:310: void
PushMessagingAPI::InitializePushMessagingEventRouterForTest() {
If it's common for tests to trigger OnListenerAdded, I'd prefer that this
function be called explicitly to create the router for tests that don't trigger
OnListenerAdded, unless that's particularly onerous.  I think it'll be clearer
in the test what's going on than having a getter that's lazy-initialized.

[Joe Thomas, 2012-12-12 02:39:27]

Addressed the review comments. Also rebased the patch.

Please review.

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:300:
ExtensionSystem::Get(profile_)->event_router()->UnregisterObserver(this);
On 2012/12/11 22:21:53, akalin wrote:
> Perhaps add a comment saying that it's okay to unregister an observer more
than
> once.

Done.

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:306:
push_messaging_event_router_.reset(new PushMessagingEventRouter(profile_));
On 2012/12/11 22:21:53, akalin wrote:
> perhaps DCHECK that push_messaging_event_router_ is NULL before resetting
(since
> we expect this to be called at most once, right?)

Done.

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:310: void
PushMessagingAPI::InitializePushMessagingEventRouterForTest() {
On 2012/12/11 22:23:41, akalin wrote:
> If it's common for tests to trigger OnListenerAdded, I'd prefer that this
> function be called explicitly to create the router for tests that don't
trigger
> OnListenerAdded, unless that's particularly onerous.  I think it'll be clearer
> in the test what's going on than having a getter that's lazy-initialized.

Done.

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:311:
push_messaging_event_router_.reset(new PushMessagingEventRouter(profile_));
On 2012/12/11 22:21:53, akalin wrote:
> can probably just call OnListenerAdded() directly here.

OnListenerAdded() takes a parameter and we don't know how to construct it here.

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:123: // For
testing purpose.
On 2012/12/11 22:07:27, Yoyo Zhou wrote:
> nit: purposes

Done.

[akalin, 2012-12-12 02:41:01]

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:311:
push_messaging_event_router_.reset(new PushMessagingEventRouter(profile_));
On 2012/12/12 02:39:27, Joe Thomas wrote:
> On 2012/12/11 22:21:53, akalin wrote:
> > can probably just call OnListenerAdded() directly here.
> 
> OnListenerAdded() takes a parameter and we don't know how to construct it
here.

Can you then add a private helper function like:

void InitializeEventRouter() {
}

and call it from OnListenerAdded() and InitializeEventRouterForTest()? (I think
the shorter name works fine, also)

[Joe Thomas, 2012-12-12 03:13:49]

Please review. Thanks

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/11496004/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:311:
push_messaging_event_router_.reset(new PushMessagingEventRouter(profile_));
On 2012/12/12 02:41:01, akalin wrote:
> On 2012/12/12 02:39:27, Joe Thomas wrote:
> > On 2012/12/11 22:21:53, akalin wrote:
> > > can probably just call OnListenerAdded() directly here.
> > 
> > OnListenerAdded() takes a parameter and we don't know how to construct it
> here.
> 
> Can you then add a private helper function like:
> 
> void InitializeEventRouter() {
> }
> 
> and call it from OnListenerAdded() and InitializeEventRouterForTest()? (I
think
> the shorter name works fine, also)

Done.

[akalin, 2012-12-12 21:39:15]

LGTM

https://chromiumcodereview.appspot.com/11496004/diff/36002/chrome/browser/ext...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://chromiumcodereview.appspot.com/11496004/diff/36002/chrome/browser/ext...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:299: // It's
safe to call UnregisterObserver more than once.
This comment could be clearer.  How about:

// UnregisterObserver() may have already been called in InitializeEventRouter(),
but it is safe to call it more than once.

https://chromiumcodereview.appspot.com/11496004/diff/36002/chrome/browser/ext...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

https://chromiumcodereview.appspot.com/11496004/diff/36002/chrome/browser/ext...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:126: private:
newline before private:

https://chromiumcodereview.appspot.com/11496004/diff/36002/chrome/browser/ext...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:129: Profile*
profile_;
do:

Profile* const profile_;

https://chromiumcodereview.appspot.com/11496004/diff/36002/chrome/browser/ext...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:130: //
Created lazily upon OnListenerAdded.
I think you can just say 'Created in OnListenerAdded.'

https://chromiumcodereview.appspot.com/11496004/diff/36002/chrome/browser/ext...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:131:
scoped_ptr<PushMessagingEventRouter> push_messaging_event_router_;
Add:

DISALLOW_COPY_AND_ASSIGN(PushMessagingAPI);

at the end of the class declaration.  Be sure to have an empty line before it

[Joe Thomas, 2012-12-13 08:34:43]

Thanks for the review.

https://codereview.chromium.org/11496004/diff/36002/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/11496004/diff/36002/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:299: // It's
safe to call UnregisterObserver more than once.
On 2012/12/12 21:39:15, akalin wrote:
> This comment could be clearer.  How about:
> 
> // UnregisterObserver() may have already been called in
InitializeEventRouter(),
> but it is safe to call it more than once.

Done.

https://codereview.chromium.org/11496004/diff/36002/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.h (right):

https://codereview.chromium.org/11496004/diff/36002/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:126: private:
On 2012/12/12 21:39:15, akalin wrote:
> newline before private:

Done.

https://codereview.chromium.org/11496004/diff/36002/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:129: Profile*
profile_;
On 2012/12/12 21:39:15, akalin wrote:
> do:
> 
> Profile* const profile_;

Done.

https://codereview.chromium.org/11496004/diff/36002/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:130: //
Created lazily upon OnListenerAdded.
On 2012/12/12 21:39:15, akalin wrote:
> I think you can just say 'Created in OnListenerAdded.'

Done.

https://codereview.chromium.org/11496004/diff/36002/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.h:131:
scoped_ptr<PushMessagingEventRouter> push_messaging_event_router_;
On 2012/12/12 21:39:15, akalin wrote:
> Add:
> 
> DISALLOW_COPY_AND_ASSIGN(PushMessagingAPI);
> 
> at the end of the class declaration.  Be sure to have an empty line before it

Done.

[commit-bot: I haz the power, 2012-12-13 08:35:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11496004/44001

[commit-bot: I haz the power, 2012-12-13 13:03:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11496004/44001

[commit-bot: I haz the power, 2012-12-13 14:05:16]

Change committed as 172876