Lazy initialization for PushMessagingEventRouter

chrome/browser/extensions/api/push_messaging/push_messaging_api.cc

Index: chrome/browser/extensions/api/push_messaging/push_messaging_api.cc
diff --git a/chrome/browser/extensions/api/push_messaging/push_messaging_api.cc b/chrome/browser/extensions/api/push_messaging/push_messaging_api.cc
index d92305af16281e95976783f8079680ab16237206..07144329bfe5e41fe636180e0cd592b8f825d6b3 100644
--- a/chrome/browser/extensions/api/push_messaging/push_messaging_api.cc
+++ b/chrome/browser/extensions/api/push_messaging/push_messaging_api.cc
@@ -10,6 +10,7 @@
 #include "base/logging.h"
 #include "base/string_number_conversions.h"
 #include "base/values.h"
+#include "chrome/browser/extensions/api/push_messaging/push_messaging_api_factory.h"
 #include "chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h"
 #include "chrome/browser/extensions/event_names.h"
 #include "chrome/browser/extensions/event_router.h"
@@ -75,15 +76,6 @@ PushMessagingEventRouter::PushMessagingEventRouter(Profile* profile)
 
 PushMessagingEventRouter::~PushMessagingEventRouter() {}
 
-void PushMessagingEventRouter::Shutdown() {
-  // We need an explicit Shutdown() due to the dependencies among the various
-  // ProfileKeyedServices. ProfileSyncService depends on ExtensionSystem, so
-  // it is destroyed before us in the destruction phase of Profile shutdown.
-  // As a result, we need to drop any references to it in the Shutdown() phase
-  // instead.
-  handler_.reset();
-}
-
 void PushMessagingEventRouter::SetMapperForTest(
     scoped_ptr<PushMessagingInvalidationMapper> mapper) {
   handler_ = mapper.Pass();
@@ -290,4 +282,39 @@ void PushMessagingGetChannelIdFunction::OnObfuscatedGaiaIdFetchFailure(
   ReportResult(std::string(), error_text);
 }
 
+PushMessagingAPI::PushMessagingAPI(Profile* profile)
+    : profile_(profile) {
+  ExtensionSystem::Get(profile_)->event_router()->RegisterObserver(
+      this, event_names::kOnPushMessage);
+}
+
+PushMessagingAPI::~PushMessagingAPI() {
+}
+
+// static
+PushMessagingAPI* PushMessagingAPI::Get(Profile* profile) {
+  return PushMessagingAPIFactory::GetForProfile(profile);
+}
+
+void PushMessagingAPI::Shutdown() {
+  ExtensionSystem::Get(profile_)->event_router()->UnregisterObserver(this);

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

Perhaps add a comment saying that it's okay to unregister an observer more than
once.

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

Done.

+  push_messaging_event_router_.reset();
+}
+
+void PushMessagingAPI::OnListenerAdded(
+    const extensions::EventListenerInfo& details) {
+  push_messaging_event_router_.reset(new PushMessagingEventRouter(profile_));

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

perhaps DCHECK that push_messaging_event_router_ is NULL before resetting (since
we expect this to be called at most once, right?)

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

Done.

+  ExtensionSystem::Get(profile_)->event_router()->UnregisterObserver(this);
+}
+
+void PushMessagingAPI::InitializePushMessagingEventRouterForTest() {

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

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.

[akalin, 2012/12/11 22:23:41]

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]

Done.

+  push_messaging_event_router_.reset(new PushMessagingEventRouter(profile_));

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

can probably just call OnListenerAdded() directly here.

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

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

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

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]

Done.

+  ExtensionSystem::Get(profile_)->event_router()->UnregisterObserver(this);
+}
+
+PushMessagingEventRouter*
+    PushMessagingAPI::GetPushMessagingEventRouterForTest() {
+  return push_messaging_event_router_.get();
+}
+
 }  // namespace extensions