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

chrome/browser/extensions/extension_gcm_app_handler.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_gcm_app_handler.h"
 
 #include "base/bind.h"
 #include "base/lazy_instance.h"
 #include "base/location.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ skipping 75 matching lines @@
     const std::string& app_id,
     const gcm::GCMClient::SendErrorDetails& send_error_details) {
 #if !defined(OS_ANDROID)
   js_event_router_->OnSendError(app_id, send_error_details);
 #endif
 }
 
 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(
     content::BrowserContext* browser_context,
     const Extension* extension) {
   if (IsGCMPermissionEnabled(extension)) {
     GetGCMDriver()->Unregister(
         extension->id(),
         base::Bind(&ExtensionGCMAppHandler::OnUnregisterCompleted,
                    weak_factory_.GetWeakPtr(),
                    extension->id()));
-    GetGCMDriver()->RemoveAppHandler(extension->id());
+    RemoveAppHandler(extension->id());
   }
 }
 
 gcm::GCMDriver* ExtensionGCMAppHandler::GetGCMDriver() const {
   return gcm::GCMProfileServiceFactory::GetForProfile(profile_)->driver();
 }
 
 void ExtensionGCMAppHandler::OnUnregisterCompleted(
     const std::string& app_id, gcm::GCMClient::Result result) {
   // 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