Push API: Refactor and fix unsubscribe API

chrome/browser/push_messaging/push_messaging_service_impl.h

Index: chrome/browser/push_messaging/push_messaging_service_impl.h
diff --git a/chrome/browser/push_messaging/push_messaging_service_impl.h b/chrome/browser/push_messaging/push_messaging_service_impl.h
index 1203d68b86a40279ee3a6a21837ae6d6cc42cc3e..afb3110150a0c3dc65b094c87471a748e77affcb 100644
--- a/chrome/browser/push_messaging/push_messaging_service_impl.h
+++ b/chrome/browser/push_messaging/push_messaging_service_impl.h
@@ -187,30 +187,40 @@ class PushMessagingServiceImpl : public content::PushMessagingService,
 
   // Unsubscribe methods -------------------------------------------------------
 
-  void Unsubscribe(const std::string& app_id,
-                   const std::string& sender_id,
-                   const content::PushMessagingService::UnregisterCallback&);
+  // |origin|, |service_worker_registration_id| and |app_id| should be provided
+  // whenever they can be obtained. It's valid for |origin| to be empty and
+  // |service_worker_registration_id| to be kInvalidServiceWorkerRegistrationId,
+  // or for app_id to be empty, but not both at once.
+  void UnsubscribeInternal(
+      content::PushUnregistrationReason reason,
+      const GURL& origin,
+      int64_t service_worker_registration_id,
+      const std::string& app_id,
+      const std::string& sender_id,
+      const content::PushMessagingService::UnregisterCallback& callback);

[Peter Beverloo, 2016/09/30 14:26:07]

q: could we use std::move() for the callback as opposed to pass by ref?

[johnme, 2016/09/30 17:02:10]

Maybe. But I think I'd rather be consistent with the rest of the class for now,
and we can bulk update these later. Otherwise it'll be hard to remember when to
wrap parameters with std::move.

+
+  void DidClearPushSubscriptionId(
+      content::PushUnregistrationReason reason,
+      const std::string& app_id,
+      const std::string& sender_id,
+      const content::PushMessagingService::UnregisterCallback& callback);
 
   void DidDeleteID(const std::string& app_id,
                    bool was_subscribed,
                    const content::PushMessagingService::UnregisterCallback&,
                    instance_id::InstanceID::Result result);
 
-  void DidUnsubscribeInstanceID(
-      const std::string& app_id,
+  void DidUnsubscribe(
+      const std::string& app_id_if_was_instance_id,

[Peter Beverloo, 2016/09/30 14:26:07]

nit: if_was -> when?

[johnme, 2016/09/30 17:02:10]

Done.

       bool was_subscribed,
-      const content::PushMessagingService::UnregisterCallback&,
-      instance_id::InstanceID::Result result);
-
-  void DidUnsubscribe(bool was_subscribed,
-                      const content::PushMessagingService::UnregisterCallback&,
-                      gcm::GCMClient::Result result);
+      const content::PushMessagingService::UnregisterCallback& callback,
+      content::PushUnregistrationStatus status);
 
   // OnContentSettingChanged methods -------------------------------------------
 
   void UnsubscribeBecausePermissionRevoked(
       const PushMessagingAppIdentifier& app_identifier,
-      const base::Closure& closure,
+      const content::PushMessagingService::UnregisterCallback& callback,
       const std::string& sender_id,
       bool success,
       bool not_found);