Push API: Fix unsubscribing from GCM on Android

components/gcm_driver/gcm_driver.h

Index: components/gcm_driver/gcm_driver.h
diff --git a/components/gcm_driver/gcm_driver.h b/components/gcm_driver/gcm_driver.h
index 04fb53660dbd7db2ec8ee9603c895b46c1baab50..29dd9ee28767108cd8ca629e766691773a805cbd 100644
--- a/components/gcm_driver/gcm_driver.h
+++ b/components/gcm_driver/gcm_driver.h
@@ -48,12 +48,22 @@ class GCMDriver {
                 const std::vector<std::string>& sender_ids,
                 const RegisterCallback& callback);
 
-  // Unregisters an app from using GCM.
+  // Unregisters an app from using GCM. Only works on non-Android; use
+  // UnregisterWithSenderId instead on Android.
   // |app_id|: application ID.
   // |callback|: to be called once the asynchronous operation is done.
   void Unregister(const std::string& app_id,
                   const UnregisterCallback& callback);
 
+  // Unregisters an app from using GCM, explicitly providing a single sender ID.

[jianli, 2015/02/17 19:17:39]

Please comment that this only applies to Android. Also please add a TODO saying
that we're going to switch to using unsubscribe.

[johnme, 2015/02/17 21:24:07]

Done.

+  // (Use normal Unregister if you registered with multiple sender IDs).

[Peter Beverloo, 2015/02/17 18:49:36]

nit: I don't think this line adds much value.

Instead, it would be worthwhile to clarify how the two are functionally
different. Unregister() removes *all* registrations for |app_id|, whereas
UnregisterWithSenderId() only removes the one matching the combination of
[sender_id, app_id], right?

[johnme, 2015/02/17 21:24:07]

Done.

+  // |app_id|: application ID.
+  // |sender_id|: the sender ID that was passed when registering.
+  // |callback|: to be called once the asynchronous operation is done.
+  void UnregisterWithSenderId(const std::string& app_id,
+                              const std::string& sender_id,
+                              const UnregisterCallback& callback);
+
   // Sends a message to a given receiver.
   // |app_id|: application ID.
   // |receiver_id|: registration ID of the receiver party.
@@ -144,7 +154,8 @@ class GCMDriver {
                             const std::vector<std::string>& sender_ids) = 0;
 
   // Platform-specific implementation of Unregister.
-  virtual void UnregisterImpl(const std::string& app_id) = 0;
+  virtual void UnregisterImpl(const std::string& app_id,
+                              const std::string* maybe_sender_id) = 0;

[Peter Beverloo, 2015/02/17 18:49:36]

nit: |maybe_sender_id| is a const pointer, it's clear that it's not meant to be
modified. Perhaps document here that the value can be a nullptr and just call it
|sender_id|?

[Peter Beverloo, 2015/02/17 18:49:36]

Since behavior of Android and non-Android is different, perhaps it could be
worth considering having two platform-specific implementations, and having a
NOTREACHED() in the Android one for UnregisterImpl()?

[jianli, 2015/02/17 19:17:39]

Yes please have two platform-specific implementations since adding an extra
argument here is kind of confusing. I think it is simpler to add
UnregisterWithSenderIdImpl with empty implementation here. Please comment that
this is for Android only and it will be changed later.
  virtual void UnregisterWithSenderIdImpl(...) {}

[johnme, 2015/02/17 21:24:07]

Done.

[johnme, 2015/02/17 21:24:07]

Done.

[johnme, 2015/02/17 21:24:07]

Done.

 
   // Platform-specific implementation of Send.
   virtual void SendImpl(const std::string& app_id,
@@ -170,6 +181,11 @@ class GCMDriver {
   void ClearCallbacks();
 
  private:
+  // Common code shared by Unregister and UnregisterWithSenderId.
+  void UnregisterInternal(const std::string& app_id,
+                          const std::string* maybe_sender_id,
+                          const UnregisterCallback& callback);
+
   // Called after unregistration completes in order to trigger the pending
   // registration.
   void RegisterAfterUnregister(