Don't stop and restart GCM when extension/app is being updated

chrome/browser/extensions/extension_gcm_app_handler.cc

Index: chrome/browser/extensions/extension_gcm_app_handler.cc
diff --git a/chrome/browser/extensions/extension_gcm_app_handler.cc b/chrome/browser/extensions/extension_gcm_app_handler.cc
index c55111e8a28d8bad3d11748d9c7f51462f15b48c..35396d5f6b5125d1552f4a8fd12174be8cfe14aa 100644
--- a/chrome/browser/extensions/extension_gcm_app_handler.cc
+++ b/chrome/browser/extensions/extension_gcm_app_handler.cc
@@ -93,16 +93,23 @@ void ExtensionGCMAppHandler::OnSendError(
 void ExtensionGCMAppHandler::OnExtensionLoaded(
     content::BrowserContext* browser_context,
     const Extension* extension) {
-  if (IsGCMPermissionEnabled(extension))
-    GetGCMDriver()->AddAppHandler(extension->id(), this);
+  // See OnExtensionUnloaded for the reason for checking if the app handler
+  // has already been added.
+  if (IsGCMPermissionEnabled(extension) &&
+      !GetGCMDriver()->app_handlers().count(extension->id()))
+    AddAppHandler(extension->id());
 }
 
 void ExtensionGCMAppHandler::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const Extension* extension,
     UnloadedExtensionInfo::Reason reason) {
-  if (IsGCMPermissionEnabled(extension))
-    GetGCMDriver()->RemoveAppHandler(extension->id());
+  // When the extension is being updated, it will be first unloaded and then
+  // loaded again. We don't want to remove and re-add the app handler that
+  // could cause the GCM service being stopped and restarted unnecessarily.
+  if (IsGCMPermissionEnabled(extension) &&
+      reason != UnloadedExtensionInfo::REASON_UPDATE)
+    RemoveAppHandler(extension->id());

[fgorski, 2014/06/12 21:38:32]

If we are unlucky enough to get a message for the extension/app while it is
unloaded for that reason. What happens? Do we need to update the code
dispatching the message, or would you rather hold on to it until the extension
is added back?

[jianli, 2014/06/12 21:48:30]

We hit DCHECK_EQ(LOADING, state_) in GCMClientImpl::OnLoadCompleted due to that
GCMClient is started and then stopped immediately. This underlying problem is
fixed by https://codereview.chromium.org/327263003/.

In addition to this fix, I think we should not stop and restart the GCM when the
extension/app is being updated.

[fgorski, 2014/06/13 15:20:21]

I am with you on not restarting GCM connection.

What I mean by "what happens?" is: "what happens to the message?"
Will it get delivered to the app/ext or not?

[jianli, 2014/06/17 00:41:56]

The message routing will not be interrupted since unload and reloading are
happening in a single function.

I've found out a problem with previous approach. I've updated the code to
address this.

 }
 
 void ExtensionGCMAppHandler::OnExtensionUninstalled(
@@ -114,7 +121,7 @@ void ExtensionGCMAppHandler::OnExtensionUninstalled(
         base::Bind(&ExtensionGCMAppHandler::OnUnregisterCompleted,
                    weak_factory_.GetWeakPtr(),
                    extension->id()));
-    GetGCMDriver()->RemoveAppHandler(extension->id());
+    RemoveAppHandler(extension->id());
   }
 }
 
@@ -127,4 +134,12 @@ void ExtensionGCMAppHandler::OnUnregisterCompleted(
   // Nothing to do.
 }
 
+void ExtensionGCMAppHandler::AddAppHandler(const std::string& app_id) {
+  GetGCMDriver()->AddAppHandler(app_id, this);
+}
+
+void ExtensionGCMAppHandler::RemoveAppHandler(const std::string& app_id) {
+  GetGCMDriver()->RemoveAppHandler(app_id);
+}
+
 }  // namespace extensions