Push API: Fix unsubscribing from GCM on Android

content/browser/push_messaging/push_messaging_message_filter.cc

Index: content/browser/push_messaging/push_messaging_message_filter.cc
diff --git a/content/browser/push_messaging/push_messaging_message_filter.cc b/content/browser/push_messaging/push_messaging_message_filter.cc
index ab3b96af9d6cb10083f7acabdfbd1d8758274ef2..bd67fc9c8cfeeb6c3dab9611035295f93b61e51d 100644
--- a/content/browser/push_messaging/push_messaging_message_filter.cc
+++ b/content/browser/push_messaging/push_messaging_message_filter.cc
@@ -367,18 +367,66 @@ void PushMessagingMessageFilter::OnUnregister(
   service_worker_context_->context()->storage()->GetUserData(
       service_worker_registration_id,
       kPushRegistrationIdServiceWorkerKey,
-      base::Bind(&PushMessagingMessageFilter::DoUnregister,
+      base::Bind(&PushMessagingMessageFilter::UnregisterWithPushRegistrationId,
                  weak_factory_io_to_io_.GetWeakPtr(),
                  request_id,
                  service_worker_registration_id,
                  service_worker_registration->pattern().GetOrigin()));
 }
 
-void PushMessagingMessageFilter::DoUnregister(
+void PushMessagingMessageFilter::UnregisterWithPushRegistrationId(
     int request_id,
     int64 service_worker_registration_id,
     const GURL& requesting_origin,
-    const std::string& push_registration_id,
+    const std::string& push_registration_id,  // Unused, we just want the status
+    ServiceWorkerStatusCode service_worker_status) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  switch (service_worker_status) {
+    case SERVICE_WORKER_OK:
+      service_worker_context_->context()->storage()->GetUserData(
+          service_worker_registration_id,
+          kSenderIdServiceWorkerKey,
+          base::Bind(
+              &PushMessagingMessageFilter::UnregisterWithSenderId,
+              weak_factory_io_to_io_.GetWeakPtr(),
+              request_id,
+              service_worker_registration_id,
+              requesting_origin));
+      return;
+    case SERVICE_WORKER_ERROR_NOT_FOUND:
+      // We did not find a registration, stop here and notify the renderer that
+      // it was a success even though we did not unregister.
+      DidUnregister(request_id,
+                    PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED);
+      return;
+    case SERVICE_WORKER_ERROR_FAILED:
+      DidUnregister(request_id,
+                    PUSH_UNREGISTRATION_STATUS_UNKNOWN_ERROR);
+      return;
+    case SERVICE_WORKER_ERROR_ABORT:
+    case SERVICE_WORKER_ERROR_START_WORKER_FAILED:
+    case SERVICE_WORKER_ERROR_PROCESS_NOT_FOUND:
+    case SERVICE_WORKER_ERROR_EXISTS:
+    case SERVICE_WORKER_ERROR_INSTALL_WORKER_FAILED:
+    case SERVICE_WORKER_ERROR_ACTIVATE_WORKER_FAILED:
+    case SERVICE_WORKER_ERROR_IPC_FAILED:
+    case SERVICE_WORKER_ERROR_NETWORK:
+    case SERVICE_WORKER_ERROR_SECURITY:
+    case SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED:
+    case SERVICE_WORKER_ERROR_STATE:
+      NOTREACHED() << "Got unexpected error code: " << service_worker_status
+                   << " " << ServiceWorkerStatusToString(service_worker_status);
+      DidUnregister(request_id, PUSH_UNREGISTRATION_STATUS_UNKNOWN_ERROR);
+      return;
+  }
+}
+
+void PushMessagingMessageFilter::UnregisterWithSenderId(

[jianli, 2015/02/12 20:03:31]

Is this going to be called only from line 391 at
UnregisterWithPushRegistrationId? If so, why do we need to pass and check the
status?

[johnme, 2015/02/13 19:11:56]

Yes, this callback is only used for the GetUserData call on line 387. I'm not
sure what you mean about passing and checking the status - this method's
service_worker_status parameter is added by GetUserData to indicate the result
of the asynchronous call, and we need to check that it succeeded as well (even
though it's unlikely that this one would fail if the previous one succeeded).

+    int request_id,
+    int64 service_worker_registration_id,
+    const GURL& requesting_origin,
+    const std::string& sender_id,
     ServiceWorkerStatusCode service_worker_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
@@ -390,7 +438,8 @@ void PushMessagingMessageFilter::DoUnregister(
                      this,
                      request_id,
                      service_worker_registration_id,
-                     requesting_origin));
+                     requesting_origin,
+                     sender_id));
       return;
     case SERVICE_WORKER_ERROR_NOT_FOUND:
       // We did not find a registration, stop here and notify the renderer that
@@ -423,14 +472,16 @@ void PushMessagingMessageFilter::DoUnregister(
 void PushMessagingMessageFilter::UnregisterFromService(
     int request_id,
     int64 service_worker_registration_id,
-    const GURL& requesting_origin) {
+    const GURL& requesting_origin,
+    const std::string& sender_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!service()) {
     DidUnregister(request_id, PUSH_UNREGISTRATION_STATUS_UNKNOWN_ERROR);
     return;
   }
 
-  service()->Unregister(requesting_origin, service_worker_registration_id,
+  service()->Unregister(
+      requesting_origin, service_worker_registration_id, sender_id,
       base::Bind(&PushMessagingMessageFilter::DidUnregisterFromService,
                  weak_factory_ui_to_ui_.GetWeakPtr(),
                  request_id,