Push API: Fix unsubscribing from GCM on Android

Push API: Fix unsubscribing from GCM on Android

We were calling Android GCM's unregister API, which clears all of an app's subscriptions (all sender ids and subtypes). Since from Android's point of view, the app is Chrome, if any webapp unsubscribed from push messaging, all webapps would be unsubscribed!

Instead now we call the unsubscribe API, which only unsubscribes a specific (sender_id,subtype) pair (where the subtype corresponds to GCMDriver's app_id). So now if a Service Worker calls pushSubscription.unsubscribe(), it'll only unsubscribe that SW's push subscription.

In order for this to work, we now also call the corresponding subscribe API (accepting a single sender_id) instead of register.

GoogleCloudMessagingV2.java is still temporary code, that will be removed once we switch to the Google Play Services client library.

BUG=457374
Committed: https://crrev.com/07b355ac6423c7957c87c66bd0f36220ca2aeaa2
Cr-Commit-Position: refs/heads/master@{#317062}

[johnme, 2015-02-10 20:28:40]

johnme@chromium.org changed reviewers:
+ fgorski@chromium.org

[jianli, 2015-02-10 23:19:25]

jianli@chromium.org changed reviewers:
+ jianli@chromium.org

[jianli, 2015-02-10 23:19:25]

https://codereview.chromium.org/914693002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/914693002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/gcm/gcm_api.cc:139: LOG(FATAL) <<
"GcmUnregisterFunction is not implemented on Android";
I think this extension related file is never linked into Android, right?

https://codereview.chromium.org/914693002/diff/1/components/gcm_driver/gcm_dr...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/914693002/diff/1/components/gcm_driver/gcm_dr...
components/gcm_driver/gcm_driver_android.cc:173: CHECK(sender_ids.size() == 1);
Is web push messaging the only client for Android? Do we have other cases
multiple sender IDs could be used on Android?

nit: use CHECK_EQ

[johnme, 2015-02-12 19:15:25]

Replied to Jian's review comments, and made various fixes so this patch now
compiles and passes tests.

https://codereview.chromium.org/914693002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/914693002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/gcm/gcm_api.cc:139: LOG(FATAL) <<
"GcmUnregisterFunction is not implemented on Android";
On 2015/02/10 23:19:25, jianli wrote:
> I think this extension related file is never linked into Android, right?

I think that's right, but I wanted there to be a clear warning if it ever gets
ported to Android.

https://codereview.chromium.org/914693002/diff/1/components/gcm_driver/gcm_dr...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/914693002/diff/1/components/gcm_driver/gcm_dr...
components/gcm_driver/gcm_driver_android.cc:173: CHECK(sender_ids.size() == 1);
On 2015/02/10 23:19:25, jianli wrote:
> Is web push messaging the only client for Android? Do we have other cases
> multiple sender IDs could be used on Android?

Push API is the only client I can find for GCMDriverAndroid. In any case, the
only GCMDriver client that uses multiple sender IDs is the chrome.gcm
Apps/Extensions API, so this should be fine.

> nit: use CHECK_EQ

Done (and below).

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

https://codereview.chromium.org/914693002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/914693002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/gcm/gcm_api.cc:139: LOG(FATAL) <<
"GcmUnregisterFunction is not implemented on Android";
On 2015/02/12 19:15:25, johnme wrote:
> On 2015/02/10 23:19:25, jianli wrote:
> > I think this extension related file is never linked into Android, right?
> 
> I think that's right, but I wanted there to be a clear warning if it ever gets
> ported to Android.

If this is your purpose, you'd better use #error directive to fail the
compilation, instead of logging. And probably you do this at the very beginning.

https://codereview.chromium.org/914693002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_gcm_app_handler.cc (right):

https://codereview.chromium.org/914693002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_gcm_app_handler.cc:136: LOG(FATAL) <<
"Unregistering from GCM requires a sender_id on Android";
ditto.

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/an...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GoogleCloudMessagingV2.java
(right):

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GoogleCloudMessagingV2.java:119:
// Use the register servlet, with 'delete=true'
nit: end with period

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GoogleCloudMessagingV2.java:120:
// Registration service returns a registration_id on success - or an error code
ditto

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver.h:53: // |sender_ids|: list of IDs of the
sender IDs that were passed when
"IDs of the sender IDs" seems to be a bit redundant. How about "list of the
sender IDs"?

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver.h:54: //               registering. On desktop
these are ignored.
Avoid mentioning desktop. Probably non-Android is better.

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_desktop.cc:526: const std::vector<std::string>&
unused) {
Probably it is a bit better to write this as
  const std::vector<std::string>& /*sender_ids*/

https://codereview.chromium.org/914693002/diff/20001/content/browser/push_mes...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/914693002/diff/20001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:425: void
PushMessagingMessageFilter::UnregisterWithSenderId(
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]

Addressed Jian's comments - thanks :)

https://codereview.chromium.org/914693002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/914693002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/gcm/gcm_api.cc:139: LOG(FATAL) <<
"GcmUnregisterFunction is not implemented on Android";
On 2015/02/12 20:03:30, jianli wrote:
> On 2015/02/12 19:15:25, johnme wrote:
> > On 2015/02/10 23:19:25, jianli wrote:
> > > I think this extension related file is never linked into Android, right?
> > 
> > I think that's right, but I wanted there to be a clear warning if it ever
gets
> > ported to Android.
> 
> If this is your purpose, you'd better use #error directive to fail the
> compilation, instead of logging. And probably you do this at the very
beginning.

Done (not sure what you meant by at the very beginning - I've moved it to the
beginning of the method, but it seems better to have it here than at the
beginning of the file, since this is the reason it's incompatible with Android).

https://codereview.chromium.org/914693002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_gcm_app_handler.cc (right):

https://codereview.chromium.org/914693002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_gcm_app_handler.cc:136: LOG(FATAL) <<
"Unregistering from GCM requires a sender_id on Android";
On 2015/02/12 20:03:30, jianli wrote:
> ditto.

Done.

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/an...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GoogleCloudMessagingV2.java
(right):

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GoogleCloudMessagingV2.java:119:
// Use the register servlet, with 'delete=true'
On 2015/02/12 20:03:30, jianli wrote:
> nit: end with period

Done.

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GoogleCloudMessagingV2.java:120:
// Registration service returns a registration_id on success - or an error code
On 2015/02/12 20:03:30, jianli wrote:
> ditto

Done.

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver.h:53: // |sender_ids|: list of IDs of the
sender IDs that were passed when
On 2015/02/12 20:03:30, jianli wrote:
> "IDs of the sender IDs" seems to be a bit redundant. How about "list of the
> sender IDs"?

Done.

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver.h:54: //               registering. On desktop
these are ignored.
On 2015/02/12 20:03:30, jianli wrote:
> Avoid mentioning desktop. Probably non-Android is better.

Done.

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/gc...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/914693002/diff/20001/components/gcm_driver/gc...
components/gcm_driver/gcm_driver_desktop.cc:526: const std::vector<std::string>&
unused) {
On 2015/02/12 20:03:31, jianli wrote:
> Probably it is a bit better to write this as
>   const std::vector<std::string>& /*sender_ids*/

Done.

https://codereview.chromium.org/914693002/diff/20001/content/browser/push_mes...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/914693002/diff/20001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:425: void
PushMessagingMessageFilter::UnregisterWithSenderId(
On 2015/02/12 20:03:31, jianli wrote:
> Is this going to be called only from line 391 at
> UnregisterWithPushRegistrationId? If so, why do we need to pass and check the
> status?

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).

[johnme, 2015-02-13 22:05:04]

johnme@chromium.org changed reviewers:
+ avi@chromium.org

[johnme, 2015-02-13 22:05:04]

avi@chromium.org: Please review changes in
content/public/browser/push_messaging_service.h

[jianli, 2015-02-13 22:33:16]

Filip and I have discussed on the approach to fix this bug by adding sender ID
to unregister. We think we'd better rely on new unsubscribe method we're
planning to add. But since the discussion on adding these are still pending, we
could understand that we try to find a way to fix the bug. But instead of
twisting Unregister method which affects lots of other files, could we consider
adding a temporary method UnregisterWithSenderID to GCMDriver and have
GCMDriverAndroid implement it only? This way, we can switch to using Unsubscribe
to do the right thing more easily in the future.

https://codereview.chromium.org/914693002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/914693002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/gcm/gcm_api.cc:136: #if defined(OS_ANDROID)
gcm_api.cc is not supposed to be included for Android. So if we do want to catch
this, we'd better add these #ifdef/#error at the beginning of the file, right
below the Copyright comment.

Also the error message should say that this file should not be compiled on
Android.

https://codereview.chromium.org/914693002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_gcm_app_handler.cc (right):

https://codereview.chromium.org/914693002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_gcm_app_handler.cc:139: #error If this ever
gets ported to Android, you must pass the real sender ID.
ditto.

https://codereview.chromium.org/914693002/diff/60001/content/browser/push_mes...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/914693002/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:377: void
PushMessagingMessageFilter::UnregisterWithPushRegistrationId(
UnregisterWithPushRegistrationId and UnregisterWithSenderId are very similar
except the handling of SERVICE_WORKER_OK. Could we try to avoid duplicating most
of the code? Probably you could do something like this:

  void PushMessagingMessageFilter::UnregisterWithPushRegistrationId(...) {
    DCHECK(...);

    if (service_worker_status == SERVICE_WORKER_OK) {
      service_worker_context_->context()->storage()->GetUserData(...);
      return;
    }
    UnregisterWithSenderId(request_id, service_worker_registration_id,
requesting_origin, std::string(), service_worker_status);
  }

[Charlie Reis, 2015-02-13 23:11:16]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2015-02-13 23:11:17]

content/public LGTM.  (Didn't look at content/browser.)

[johnme, 2015-02-14 01:38:30]

New patchsets have been uploaded after l-g-t-m from creis@chromium.org

[johnme, 2015-02-14 01:48:35]

> Filip and I have discussed on the approach to fix this bug by adding sender ID
> to unregister. We think we'd better rely on new unsubscribe method we're
> planning to add. But since the discussion on adding these are still pending,
> we could understand that we try to find a way to fix the bug. But instead of
> twisting Unregister method which affects lots of other files, could we
> consider adding a temporary method UnregisterWithSenderID to GCMDriver and
> have GCMDriverAndroid implement it only? This way, we can switch to using
> Unsubscribe to do the right thing more easily in the future.

Done - I added UnregisterWithSenderID, and made GCMDriverAndroid only implement
that. GCMDriverDesktop implements both, because it was simpler/neater that way
(it simply ignores the passed sender id), and that way I don't need ifdefs in
push_messaging_service_impl.cc to decide which to use on which platform.

https://codereview.chromium.org/914693002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/914693002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/gcm/gcm_api.cc:136: #if defined(OS_ANDROID)
On 2015/02/13 22:33:16, jianli wrote:
> gcm_api.cc is not supposed to be included for Android. So if we do want to
catch
> this, we'd better add these #ifdef/#error at the beginning of the file, right
> below the Copyright comment.
> 
> Also the error message should say that this file should not be compiled on
> Android.

The patch no longer modifies this file.

https://codereview.chromium.org/914693002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_gcm_app_handler.cc (right):

https://codereview.chromium.org/914693002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_gcm_app_handler.cc:139: #error If this ever
gets ported to Android, you must pass the real sender ID.
On 2015/02/13 22:33:16, jianli wrote:
> ditto.

The patch no longer modifies this file.

https://codereview.chromium.org/914693002/diff/60001/content/browser/push_mes...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/914693002/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:377: void
PushMessagingMessageFilter::UnregisterWithPushRegistrationId(
On 2015/02/13 22:33:16, jianli wrote:
> UnregisterWithPushRegistrationId and UnregisterWithSenderId are very similar
> except the handling of SERVICE_WORKER_OK. Could we try to avoid duplicating
most
> of the code? Probably you could do something like this:
> 
>   void PushMessagingMessageFilter::UnregisterWithPushRegistrationId(...) {
>     DCHECK(...);
> 
>     if (service_worker_status == SERVICE_WORKER_OK) {
>       service_worker_context_->context()->storage()->GetUserData(...);
>       return;
>     }
>     UnregisterWithSenderId(request_id, service_worker_registration_id,
> requesting_origin, std::string(), service_worker_status);
>   }

Done (yeah, that's nicer).

[johnme, 2015-02-14 01:48:45]

johnme@chromium.org changed reviewers:
- avi@chromium.org

[Avi (use Gerrit), 2015-02-17 16:25:48]

avi@chromium.org changed reviewers:
+ avi@chromium.org

[Avi (use Gerrit), 2015-02-17 16:25:49]

lgtm

[Peter Beverloo, 2015-02-17 18:49:34]

peter@chromium.org changed reviewers:
+ peter@chromium.org

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

Mostly nits from my side. Thanks for the patch!!

https://codereview.chromium.org/914693002/diff/140001/chrome/browser/services...
File chrome/browser/services/gcm/fake_gcm_profile_service.cc (right):

https://codereview.chromium.org/914693002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/fake_gcm_profile_service.cc:72: const std::string*
/* maybe_sender_id */) {
nit: we don't warn on unused arguments, please just give it a name.

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.cc:103: 
micro nit: extra blank line

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:59: // (Use normal Unregister if you
registered with multiple sender IDs).
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?

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:158: const std::string* maybe_sender_id) = 0;
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()?

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:158: const std::string* maybe_sender_id) = 0;
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|?

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:173: CHECK_EQ(1u,
sender_ids.size());
We use DCHECK_EQ unless it has security implications. Dito on line 183.

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop.cc:526: const std::string& app_id,
const std::string* /* maybe_sender_id */) {
nit: Please name the argument.

https://codereview.chromium.org/914693002/diff/140001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/914693002/diff/140001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:381: const
std::string& push_registration_id,  // Unused, we just want the status
I find it odd that the method is called "UnregisterWithPushRegistrationId" but
that the push_registration_id is unused. Then it doesn't matter at all that
we've got a PushRegistrationId, right? Perhaps
"UnregisterGetSenderIdFromUserData" would be better?

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

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:58: // Unregisters an app from using GCM,
explicitly providing a single sender ID.
Please comment that this only applies to Android. Also please add a TODO saying
that we're going to switch to using unsubscribe.

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:158: const std::string* maybe_sender_id) = 0;
On 2015/02/17 18:49:36, Peter Beverloo wrote:
> 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()?

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:23:29]

New patchsets have been uploaded after l-g-t-m from avi@chromium.org

[johnme, 2015-02-17 21:23:32]

Address peter & jianli's review comments

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

Addressed jianli@ & peter@'s review comments - thanks :)

https://codereview.chromium.org/914693002/diff/140001/chrome/browser/services...
File chrome/browser/services/gcm/fake_gcm_profile_service.cc (right):

https://codereview.chromium.org/914693002/diff/140001/chrome/browser/services...
chrome/browser/services/gcm/fake_gcm_profile_service.cc:72: const std::string*
/* maybe_sender_id */) {
On 2015/02/17 18:49:36, Peter Beverloo wrote:
> nit: we don't warn on unused arguments, please just give it a name.

Removed.

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.cc:103: 
On 2015/02/17 18:49:36, Peter Beverloo wrote:
> micro nit: extra blank line

Done.

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:58: // Unregisters an app from using GCM,
explicitly providing a single sender ID.
On 2015/02/17 19:17:39, jianli wrote:
> Please comment that this only applies to Android. Also please add a TODO
saying
> that we're going to switch to using unsubscribe.

Done.

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:59: // (Use normal Unregister if you
registered with multiple sender IDs).
On 2015/02/17 18:49:36, Peter Beverloo wrote:
> 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?

Done.

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:158: const std::string* maybe_sender_id) = 0;
On 2015/02/17 18:49:36, Peter Beverloo wrote:
> 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|?

Done.

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:158: const std::string* maybe_sender_id) = 0;
On 2015/02/17 18:49:36, Peter Beverloo wrote:
> 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()?

Done.

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:158: const std::string* maybe_sender_id) = 0;
On 2015/02/17 19:17:39, jianli wrote:
> On 2015/02/17 18:49:36, Peter Beverloo wrote:
> > 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()?
> 
> 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(...) {}

Done.

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:173: CHECK_EQ(1u,
sender_ids.size());
On 2015/02/17 18:49:36, Peter Beverloo wrote:
> We use DCHECK_EQ unless it has security implications. Dito on line 183.

Done.

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/914693002/diff/140001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop.cc:526: const std::string& app_id,
const std::string* /* maybe_sender_id */) {
On 2015/02/17 18:49:36, Peter Beverloo wrote:
> nit: Please name the argument.

Removed.

https://codereview.chromium.org/914693002/diff/140001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/914693002/diff/140001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:381: const
std::string& push_registration_id,  // Unused, we just want the status
On 2015/02/17 18:49:36, Peter Beverloo wrote:
> I find it odd that the method is called "UnregisterWithPushRegistrationId" but
> that the push_registration_id is unused. Then it doesn't matter at all that
> we've got a PushRegistrationId, right? Perhaps
> "UnregisterGetSenderIdFromUserData" would be better?

I went with UnregisterHavingGottenPushRegistrationId and
UnregisterHavingGottenSenderId (as discussed on chat).

[Peter Beverloo, 2015-02-17 21:51:05]

push lgtm, thanks :-)

https://codereview.chromium.org/914693002/diff/160001/chrome/browser/services...
File chrome/browser/services/gcm/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/914693002/diff/160001/chrome/browser/services...
chrome/browser/services/gcm/push_messaging_browsertest.cc:631:
message.data["data"] = "shownotification-without-waituntil";
nit: pass the sender id

https://codereview.chromium.org/914693002/diff/160001/chrome/browser/services...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/914693002/diff/160001/chrome/browser/services...
chrome/browser/services/gcm/push_messaging_service_impl.cc:660: #if
defined(OS_ANDROID)
nit: leave a comment about why this difference is necessary

https://codereview.chromium.org/914693002/diff/160001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/914693002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:161: const std::string& sender_id);
All the other *Impl methods are pure virtual, should we override the non-Android
implementation and have the NOTREACHED() there?

https://codereview.chromium.org/914693002/diff/160001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/914693002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:187: {
nit: I guess we have to indent this "{" by four spaces.

[johnme, 2015-02-17 22:04:48]

New patchsets have been uploaded after l-g-t-m from peter@chromium.org

[johnme, 2015-02-17 22:05:10]

Addressed peter's nits

https://codereview.chromium.org/914693002/diff/160001/chrome/browser/services...
File chrome/browser/services/gcm/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/914693002/diff/160001/chrome/browser/services...
chrome/browser/services/gcm/push_messaging_browsertest.cc:631:
message.data["data"] = "shownotification-without-waituntil";
On 2015/02/17 21:51:05, Peter Beverloo wrote:
> nit: pass the sender id

Done.

https://codereview.chromium.org/914693002/diff/160001/chrome/browser/services...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/914693002/diff/160001/chrome/browser/services...
chrome/browser/services/gcm/push_messaging_service_impl.cc:660: #if
defined(OS_ANDROID)
On 2015/02/17 21:51:05, Peter Beverloo wrote:
> nit: leave a comment about why this difference is necessary

Done.

https://codereview.chromium.org/914693002/diff/160001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/914693002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:161: const std::string& sender_id);
On 2015/02/17 21:51:05, Peter Beverloo wrote:
> All the other *Impl methods are pure virtual, should we override the
non-Android
> implementation and have the NOTREACHED() there?

No, I think the reason Jian suggested doing it this way is so that all the tests
which subclass GCMDriver wouldn't have to implement an unnecessary/confusing
UnregisterWithSenderIdImpl method just because of Android. Once they refactor
GCM to use the subscribe/unsubscribe APIs, this will be cleaned up.

https://codereview.chromium.org/914693002/diff/160001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/914693002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:187: {
On 2015/02/17 21:51:05, Peter Beverloo wrote:
> nit: I guess we have to indent this "{" by four spaces.

Eww, no. I've wrapped the arguments to avoid this :)

[jianli, 2015-02-17 23:02:27]

lgtm

https://codereview.chromium.org/914693002/diff/180001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/914693002/diff/180001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:58: // Unregisters an app_id,sender_id pair
from using GCM. Only works on Android.
nit: "(app_id, sender)" or "app_id and sender_id"

[johnme, 2015-02-17 23:09:35]

New patchsets have been uploaded after l-g-t-m from jianli@chromium.org

[johnme, 2015-02-17 23:10:11]

https://codereview.chromium.org/914693002/diff/180001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/914693002/diff/180001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:58: // Unregisters an app_id,sender_id pair
from using GCM. Only works on Android.
On 2015/02/17 23:02:27, jianli wrote:
> nit: "(app_id, sender)" or "app_id and sender_id"

Done.

[fgorski, 2015-02-17 23:31:58]

lgtm with a nit.

John, thanks for being patient with us. I think we ended up with a pretty good
change, which is going to be easier to update in future.

https://codereview.chromium.org/914693002/diff/200001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/914693002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.cc:85: const std::string* sender_id,
nit: was passing an empty string from the version without a sender better? You
wouldn't have to use a const pointer then. Just a thought...

[johnme, 2015-02-17 23:38:20]

https://codereview.chromium.org/914693002/diff/200001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/914693002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.cc:85: const std::string* sender_id,
On 2015/02/17 23:31:58, fgorski wrote:
> nit: was passing an empty string from the version without a sender better? You
> wouldn't have to use a const pointer then. Just a thought...

That would be ambiguous though - for example on Android, if a website passes an
empty string for the sender ID, we'd end up calling the NOTREACHED()
UnregisterImpl, which would be bad.

[johnme, 2015-02-18 20:13:04]

New patchsets have been uploaded after l-g-t-m from fgorski@chromium.org

[johnme, 2015-02-18 20:13:07]

Rebase

[johnme, 2015-02-18 21:44:29]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2015-02-18 21:45:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/914693002/220001

[commit-bot: I haz the power, 2015-02-18 21:59:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-18 21:59:12]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[johnme, 2015-02-19 16:08:40]

Heads up to reviewers: I had to make quite a few changes when rebasing,
especially due to https://codereview.chromium.org/930083002; if you have any new
concerns, I'll address them asap.

[johnme, 2015-02-19 16:11:18]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2015-02-19 16:12:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/914693002/280001

[commit-bot: I haz the power, 2015-02-19 17:01:00]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-02-19 17:01:53]

Patchset 15 (id:??) landed as
https://crrev.com/07b355ac6423c7957c87c66bd0f36220ca2aeaa2
Cr-Commit-Position: refs/heads/master@{#317062}