Push API: Fix unsubscribing from GCM on Android

components/gcm_driver/gcm_driver.h

 // 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.
 
 #ifndef COMPONENTS_GCM_DRIVER_GCM_DRIVER_H_
 #define COMPONENTS_GCM_DRIVER_GCM_DRIVER_H_
 
 #include <map>
 #include <string>
 #include <vector>
@@ skipping 30 matching lines @@
   // the GCM server.
   // |app_id|: application ID.
   // |sender_ids|: list of IDs of the servers that are allowed to send the
   //               messages to the application. These IDs are assigned by the
   //               Google API Console.
   // |callback|: to be called once the asynchronous operation is done.
   void Register(const std::string& app_id,
                 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.
   // |message|: message to be sent.
   // |callback|: to be called once the asynchronous operation is done.
   void Send(const std::string& app_id,
             const std::string& receiver_id,
             const GCMClient::OutgoingMessage& message,
             const SendCallback& callback);
 
@@ skipping 70 matching lines @@
 
  protected:
   // Ensures that the GCM service starts (if necessary conditions are met).
   virtual GCMClient::Result EnsureStarted(GCMClient::StartMode start_mode) = 0;
 
   // Platform-specific implementation of Register.
   virtual void RegisterImpl(const std::string& app_id,
                             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,
                         const std::string& receiver_id,
                         const GCMClient::OutgoingMessage& message) = 0;
 
   // Runs the Register callback.
   void RegisterFinished(const std::string& app_id,
                         const std::string& registration_id,
                         GCMClient::Result result);
 
   // Runs the Unregister callback.
   void UnregisterFinished(const std::string& app_id,
                           GCMClient::Result result);
 
   // Runs the Send callback.
   void SendFinished(const std::string& app_id,
                     const std::string& message_id,
                     GCMClient::Result result);
 
   bool HasRegisterCallback(const std::string& app_id);
 
   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(
       const std::string& app_id,
       const std::vector<std::string>& normalized_sender_ids,
       const UnregisterCallback& unregister_callback,
       GCMClient::Result result);
 
   // Callback map (from app_id to callback) for Register.
   std::map<std::string, RegisterCallback> register_callbacks_;
@@ skipping 12 matching lines @@
   DefaultGCMAppHandler default_app_handler_;
 
   base::WeakPtrFactory<GCMDriver> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(GCMDriver);
 };
 
 }  // namespace gcm
 
 #endif  // COMPONENTS_GCM_DRIVER_GCM_DRIVER_H_