Make Web Push use InstanceID tokens instead of GCM registrations

Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. https://codereview.chromium.org/2111973002 adds IID subtypes on desktop
9. this patch

Also depends on
- https://codereview.chromium.org/1945753002 transactional SW userdata
- https://codereview.chromium.org/1944863002 fix sender ID storage

BUG=589461, 533498
Committed: https://crrev.com/a504573264e3021119337fdfbebc76c7f72a3510
Cr-Commit-Position: refs/heads/master@{#417323}

[johnme, 2016-04-04 12:19:42]

Description was changed from

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

BUG=589461
==========

to

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1854093002 enables InstanceID by default
5. this patch

BUG=589461
==========

[johnme, 2016-04-05 19:15:56]

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

[Peter Beverloo, 2016-04-11 15:47:56]

A few higher-level comments. I suspect it will change significantly based on the
series of CLs it depends on so I'll do a full review later.

https://codereview.chromium.org/1851423003/diff/60001/base/message_loop/messa...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1851423003/diff/60001/base/message_loop/messa...
base/message_loop/message_loop.cc:679: BindToCurrentThread();
Stray change?

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:269: std::string
endpoint_prefix = std::string(kPushMessagingEndpoint) + "/";
We cannot rely on kPushMessagingEndpoint anymore. Can we instead get everything
past the last slash?

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:546:
SubscribeSuccessfully();
It seems to me that this very much is something we'd want to ASSERT on.

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_factory.cc (left):

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_factory.cc:38:
DependsOn(gcm::GCMProfileServiceFactory::GetInstance());
We still use the GCMProfileServiceFactory in the ServiceImpl.

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_factory.cc (right):

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_factory.cc:27: return
nullptr;
+LOG(WARNING)?

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_unittest.cc (right):

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_unittest.cc:100:
base::FieldTrialList::CreateFieldTrial("InstanceID", "Enabled");
Why are you removing the enable-InstanceID-field-trail code elsewhere, but are
adding it here?

https://codereview.chromium.org/1854093002

https://codereview.chromium.org/1851423003/diff/60001/content/public/test/tes...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/1851423003/diff/60001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.h:67: NESTED_JAVA_ON_ANDROID =
0x04,
I am very concerned about further spreading availability of
NestedSystemMessageHandler. Regardless, does not belong in this CL.

[johnme, 2016-04-18 14:23:45]

Description was changed from

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1854093002 enables InstanceID by default
5. this patch

BUG=589461
==========

to

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. this patch

BUG=589461
==========

[johnme, 2016-04-21 10:58:38]

Addressed review comments, and significantly rewrote following rebase (e.g. no
longer depends on NestedMessagePumpAndroid). Thanks!

https://codereview.chromium.org/1851423003/diff/60001/base/message_loop/messa...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1851423003/diff/60001/base/message_loop/messa...
base/message_loop/message_loop.cc:679: BindToCurrentThread();
On 2016/04/11 15:47:56, Peter Beverloo wrote:
> Stray change?

No, but removed now I no longer use NestedMessagePumpAndroid.

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:269: std::string
endpoint_prefix = std::string(kPushMessagingEndpoint) + "/";
On 2016/04/11 15:47:56, Peter Beverloo wrote:
> We cannot rely on kPushMessagingEndpoint anymore. Can we instead get
everything
> past the last slash?

Done.

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:546:
SubscribeSuccessfully();
On 2016/04/11 15:47:56, Peter Beverloo wrote:
> It seems to me that this very much is something we'd want to ASSERT on.

Done (and generally cleaned this up elsewhere too).

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_factory.cc (left):

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_factory.cc:38:
DependsOn(gcm::GCMProfileServiceFactory::GetInstance());
On 2016/04/11 15:47:56, Peter Beverloo wrote:
> We still use the GCMProfileServiceFactory in the ServiceImpl.

Good point. Readded (I guess it was working only because IID happens to depend
on GCM).

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_factory.cc (right):

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_factory.cc:27: return
nullptr;
On 2016/04/11 15:47:56, Peter Beverloo wrote:
> +LOG(WARNING)?

Added a DLOG(WARNING).

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_unittest.cc (right):

https://codereview.chromium.org/1851423003/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_unittest.cc:100:
base::FieldTrialList::CreateFieldTrial("InstanceID", "Enabled");
On 2016/04/11 15:47:56, Peter Beverloo wrote:
> Why are you removing the enable-InstanceID-field-trail code elsewhere, but are
> adding it here?
> 
> https://codereview.chromium.org/1854093002

Hadn't yet rebased this. Removed.

https://codereview.chromium.org/1851423003/diff/60001/content/public/test/tes...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/1851423003/diff/60001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.h:67: NESTED_JAVA_ON_ANDROID =
0x04,
On 2016/04/11 15:47:56, Peter Beverloo wrote:
> I am very concerned about further spreading availability of
> NestedSystemMessageHandler. Regardless, does not belong in this CL.

Removed since I no longer use NestedSystemMessageHandler.

[Peter Beverloo, 2016-04-21 13:26:19]

Thanks John! This looks a lot better :-) A few questions.

https://codereview.chromium.org/1851423003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java
(right):

https://codereview.chromium.org/1851423003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:175:
return iid.mTokens.keySet().iterator().next().split(",", 3)[1];
Please document this— how should a Chromium Sheriff interpret the role of this
code when investigating a failure in any test that called
getSubscribedSenderId()?

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:81: const char
kManifestSenderId[] = "1234567890";
micro nit: Please group the constants together (i.e. move this to line 75).

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:178: // Sets
out_token to the subscription token (not including server URL).
// Calls should be wrapped in the ASSERT_NO_FATAL_FAILURE() macro.

Probably want to annotate the other methods that need this as well?

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_factory.cc (right):

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_factory.cc:28:
DLOG(WARNING) << "PushMessagingService could not be built, because "
LOG(WARNING).

When, for whatever reason, the kill switch has to be flipped I'd like to be able
to read this in logs received from users for whom "it does not work".

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_factory.cc:29:
"InstanceIDProfileService is unexpectedly disabled";
micro nits:
 (1) no comma
 (2) InstanceIDProfileService -> InstanceID

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (left):

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:716: //
(https://crbug.com/402458).
Now that we no longer rely on the sender_id I think this will actually enable us
to do something sensible in response to
ServiceWorkerContextObserver::OnRegistrationDeleted() and
ServiceWorkerContextObserver::OnStorageWiped() \o/

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:62: const char
kGCMScope[] = "GCM";
micro nit: blank line after namespace definition.

Please document this constant as well.

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:412:
PushMessagingServiceImpl::GetPermissionStatus(requesting_origin,
What's up with this call? I.e. why isn't it this->GetPermissionStatus()? Should
probably kill blink::WebPushPermissionStatus too at some point.

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:509: status =
content::PUSH_REGISTRATION_STATUS_SERVICE_ERROR;
I'm beginning to think we should consider having DLOG(ERROR) calls in aggregated
switch cases like this, which would tremendously help debugging of any sort of
issue. WDYT?

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:603:
GetInstanceIDDriver()->GetInstanceID(app_id)->DeleteID(
Now that we don't call GCMDriver::Unregister(WithSenderId), what gets rid of the
stored encryption keys in the GCM key store?

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:619: if
(callback.is_null())
While not related to your CL, this isn't true anymore. Mind updating this to:

DCHECK(!callback.is_null());

And remove the if-statement from PushMessagingServiceImpl::Unsubscribe() for
bonus points? :-)

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:682:
UnsubscribeBecausePermissionRevoked(app_identifier, barrier_closure);
Can we coalesce UnsubscribeBecausePermissionRevoked() into this method? It's
only three additional statements.

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

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/services...
chrome/browser/services/gcm/fake_gcm_profile_service.cc:26: class
CustomFakeGCMDriver : public instance_id::FakeGCMDriverForInstanceID {
Can we remove the driver override from the InstanceIDApiTest now?
(instance_id_apitest.cc:33)

https://codereview.chromium.org/1851423003/diff/80001/components/gcm_driver/a...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/FakeGoogleCloudMessagingSubscriber.java
(left):

https://codereview.chromium.org/1851423003/diff/80001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/FakeGoogleCloudMessagingSubscriber.java:18:
public class FakeGoogleCloudMessagingSubscriber implements
GoogleCloudMessagingSubscriber {
We are basically losing the only user of GCMDriverAndroid::{Register,
UnregisterWithSenderId} now, right? Do we have sufficient test coverage left of
that functionality?

https://codereview.chromium.org/1851423003/diff/80001/components/gcm_driver/i...
File
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1851423003/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:27:
public static Map<String, InstanceIDWithSubtype> sSubtypeInstances = new
HashMap<>();
Have you considered adding a public (for testing) convenience method for getting
the only registered subtype and sender Id? Something like:

// Asserts + throws that sSubtypeInstances.size() == 1
// Returns a pair of [subtype, sender Id] of the only instance.
public android.util.Pair<String, String> getOnlyInstanceIdForTesting();

That would remove the parsing complexity out of PushMessagingTest.java and moves
it much closer to the source.

https://codereview.chromium.org/1851423003/diff/80001/components/gcm_driver/i...
File components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.h
(right):

https://codereview.chromium.org/1851423003/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.h:51: const
std::string& last_registered_app_id() const {
Bikeshedding:
  last_instanceid_token_app_id?
  last_instanceid_token_authorized_entity?

To ambiguate between registration and InstanceID tokens..

[johnme, 2016-04-27 18:00:48]

Description was changed from

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. this patch

BUG=589461
==========

to

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1923953002 adds crypto integration
7. this patch

BUG=589461
==========

[johnme, 2016-05-06 12:40:06]

Description was changed from

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1923953002 adds crypto integration
7. this patch

BUG=589461
==========

to

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch

Also depends on https://codereview.chromium.org/1944863002

BUG=589461
==========

[johnme, 2016-05-06 12:47:21]

Description was changed from

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch

Also depends on https://codereview.chromium.org/1944863002

BUG=589461
==========

to

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch

Also depends on https://codereview.chromium.org/1945753002 and
https://codereview.chromium.org/1944863002.

BUG=589461
==========

[johnme, 2016-05-06 12:47:39]

Description was changed from

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch

Also depends on https://codereview.chromium.org/1945753002 and
https://codereview.chromium.org/1944863002.

BUG=589461
==========

to

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch

Also depends on https://codereview.chromium.org/1945753002 and
https://codereview.chromium.org/1944863002.

BUG=589461
==========

[johnme, 2016-05-06 12:49:00]

Description was changed from

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch

Also depends on https://codereview.chromium.org/1945753002 and
https://codereview.chromium.org/1944863002.

BUG=589461
==========

to

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch

Also depends on
- https://codereview.chromium.org/1945753002 transactional SW userdata
- https://codereview.chromium.org/1944863002 fix sender ID storage

BUG=589461
==========

[johnme, 2016-05-25 14:11:51]

Addressed review comments - thanks!

https://codereview.chromium.org/1851423003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java
(right):

https://codereview.chromium.org/1851423003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:175:
return iid.mTokens.keySet().iterator().next().split(",", 3)[1];
On 2016/04/21 13:26:18, Peter Beverloo wrote:
> Please document this— how should a Chromium Sheriff interpret the role of this
> code when investigating a failure in any test that called
> getSubscribedSenderId()?

Done (I added a comment to
FakeInstanceIDWithSubtype.getSubtypeAndAuthorizedEntityOfOnlyToken explaining
reasons why there may be too many or too few IIDs/tokens).

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:81: const char
kManifestSenderId[] = "1234567890";
On 2016/04/21 13:26:18, Peter Beverloo wrote:
> micro nit: Please group the constants together (i.e. move this to line 75).

Done (moved to line 61)

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:178: // Sets
out_token to the subscription token (not including server URL).
On 2016/04/21 13:26:18, Peter Beverloo wrote:
> // Calls should be wrapped in the ASSERT_NO_FATAL_FAILURE() macro.
> 
> Probably want to annotate the other methods that need this as well?

Done.

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_factory.cc (right):

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_factory.cc:28:
DLOG(WARNING) << "PushMessagingService could not be built, because "
On 2016/04/21 13:26:18, Peter Beverloo wrote:
> LOG(WARNING).
> 
> When, for whatever reason, the kill switch has to be flipped I'd like to be
able
> to read this in logs received from users for whom "it does not work".

Done.

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_factory.cc:29:
"InstanceIDProfileService is unexpectedly disabled";
On 2016/04/21 13:26:18, Peter Beverloo wrote:
> micro nits:
>  (1) no comma
>  (2) InstanceIDProfileService -> InstanceID

Done.

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (left):

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:716: //
(https://crbug.com/402458).
On 2016/04/21 13:26:19, Peter Beverloo wrote:
> Now that we no longer rely on the sender_id I think this will actually enable
us
> to do something sensible in response to
> ServiceWorkerContextObserver::OnRegistrationDeleted() and
> ServiceWorkerContextObserver::OnStorageWiped() \o/

Yup - the future is bright! Though unfortunately, we still need the sender_id to
unsubscribe legacy GCM registrations on Android, so this isn't quite as simple.

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:62: const char
kGCMScope[] = "GCM";
On 2016/04/21 13:26:19, Peter Beverloo wrote:
> micro nit: blank line after namespace definition.
> 
> Please document this constant as well.

Done.

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:412:
PushMessagingServiceImpl::GetPermissionStatus(requesting_origin,
On 2016/04/21 13:26:19, Peter Beverloo wrote:
> What's up with this call? I.e. why isn't it this->GetPermissionStatus()?
Should
> probably kill blink::WebPushPermissionStatus too at some point.

Weird. Removed the PushMessagingServiceImpl::.

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:509: status =
content::PUSH_REGISTRATION_STATUS_SERVICE_ERROR;
On 2016/04/21 13:26:18, Peter Beverloo wrote:
> I'm beginning to think we should consider having DLOG(ERROR) calls in
aggregated
> switch cases like this, which would tremendously help debugging of any sort of
> issue. WDYT?

Done (hopefully it won't trigger too often; in theory it's possible for sloppy
sites to trigger this in a loop).

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:603:
GetInstanceIDDriver()->GetInstanceID(app_id)->DeleteID(
On 2016/04/21 13:26:18, Peter Beverloo wrote:
> Now that we don't call GCMDriver::Unregister(WithSenderId), what gets rid of
the
> stored encryption keys in the GCM key store?

848 lines later... Done (https://codereview.chromium.org/1923953002)

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:619: if
(callback.is_null())
On 2016/04/21 13:26:18, Peter Beverloo wrote:
> While not related to your CL, this isn't true anymore. Mind updating this to:
> 
> DCHECK(!callback.is_null());
> 
> And remove the if-statement from PushMessagingServiceImpl::Unsubscribe() for
> bonus points? :-)

Done.

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:682:
UnsubscribeBecausePermissionRevoked(app_identifier, barrier_closure);
On 2016/04/21 13:26:19, Peter Beverloo wrote:
> Can we coalesce UnsubscribeBecausePermissionRevoked() into this method? It's
> only three additional statements.

Not any more - I had to re-add the async GetSenderId call.

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

https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/services...
chrome/browser/services/gcm/fake_gcm_profile_service.cc:26: class
CustomFakeGCMDriver : public instance_id::FakeGCMDriverForInstanceID {
On 2016/04/21 13:26:19, Peter Beverloo wrote:
> Can we remove the driver override from the InstanceIDApiTest now?
> (instance_id_apitest.cc:33)

Done.

https://codereview.chromium.org/1851423003/diff/80001/components/gcm_driver/a...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/FakeGoogleCloudMessagingSubscriber.java
(left):

https://codereview.chromium.org/1851423003/diff/80001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/FakeGoogleCloudMessagingSubscriber.java:18:
public class FakeGoogleCloudMessagingSubscriber implements
GoogleCloudMessagingSubscriber {
On 2016/04/21 13:26:18, Peter Beverloo wrote:
> We are basically losing the only user of GCMDriverAndroid::{Register,
> UnregisterWithSenderId} now, right? Do we have sufficient test coverage left
of
> that functionality?

I re-added UnregisterWithSenderId during my backwards compatibility analysis (we
use it when unsubscribing legacy registrations).

As for GCMDriverAndroid::Register, it is indeed unused and untested now. We'll
probably start using it again once we fix https://crbug.com/600286 (unless we
start auto-expiring registrations once per week, but that seems unlikely).

https://codereview.chromium.org/1851423003/diff/80001/components/gcm_driver/i...
File
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1851423003/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:27:
public static Map<String, InstanceIDWithSubtype> sSubtypeInstances = new
HashMap<>();
On 2016/04/21 13:26:19, Peter Beverloo wrote:
> Have you considered adding a public (for testing) convenience method for
getting
> the only registered subtype and sender Id? Something like:
> 
> // Asserts + throws that sSubtypeInstances.size() == 1
> // Returns a pair of [subtype, sender Id] of the only instance.
> public android.util.Pair<String, String> getOnlyInstanceIdForTesting();
> 
> That would remove the parsing complexity out of PushMessagingTest.java and
moves
> it much closer to the source.

Done.

https://codereview.chromium.org/1851423003/diff/80001/components/gcm_driver/i...
File components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.h
(right):

https://codereview.chromium.org/1851423003/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.h:51: const
std::string& last_registered_app_id() const {
On 2016/04/21 13:26:19, Peter Beverloo wrote:
> Bikeshedding:
>   last_instanceid_token_app_id?
>   last_instanceid_token_authorized_entity?
> 
> To ambiguate between registration and InstanceID tokens..

Renamed to last_gettoken_*

[Peter Beverloo, 2016-06-02 15:53:18]

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_app_identifier.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:222:
DCHECK(app_id_.size() > kPrefixLength + suffix_length);
nit: DCHECK_GT

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:240: )));
This is a ten line DCHECK containing significant logic, the complication of this
is way too high.

Let's instead separate this out in a series of variables living on the stack.
How about the following?

===============
  // App identifiers created for InstanceID-based subscriptions have a suffix
  // allowing them to be identified as such, whilst breaking the GUID syntax.
  std::string guid = app_id_.substr(app_id_.size() - kGuidLength);
  bool is_instance_id =
      base::EndsWith(guid, kInstanceIDGuidSuffix, base::CompareCase::SENSITIVE);

  if (is_instance_id) {
    DCHECK(!base::IsValidGUID(guid));

    // Replace the suffix with valid GUID characters in order to be able to
    // validate the rest of the string.
    guid = guid.replace(guid.size() - kInstanceIDGuidSuffix,
                        kInstanceIDGuidSuffix, kInstanceIDGuidSuffix, 'C');
  }

  DCHECK(base::IsValidGUID(guid));
===============

That's incredibly much easier to read.

If you're worried about compiling that code in non-debug builds, let's stick the
entire body of this method in an "#if DCHECK_IS_ON()" block.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:338: // Create a
non-InstanceID GCM registration. We have to directly access
micro nit: no 'we' in comments. I also question the value of the final part of
this comment- the code only cares about the current (and maybe future) state of
itself.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:385: std::string*
out_token) {
This file has an inconsistency between requiring out_* values and having them be
optional. Could we do one or the other? (My preference is always requiring
them.)

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:392: size_t
min_token_length = 5;
Why 5? Why not >0? If there is no known size, let's not care about it at all?

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1186: // Push
subscriptions used to be non-InstanceID GCM registrations. We still need
nit: no 'we'

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1188:
IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, LegacyUnsubscribeSuccess) {
Can we test receiving messages for `legacy` subscriptions?

In line with the comments I shared when reviewing the GCM Driver changes, I
still have a very strong preference for avoiding the use of "legacy" in our
code.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_service_factory.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_factory.cc:31: "InstanceID
is unexpectedly disabled";
s/unexpectedly//, these things don't happen by accident :)

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_factory.cc:31: "InstanceID
is unexpectedly disabled";
Is there a watchlist or something we can subscribe to to make sure that we
immediately get informed when someone flips the kill-switch?

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:196: return
app_id.rfind(kPushMessagingAppIdentifierPrefix, 0) == 0;
Why can't we do a prefix search? (I.e. base::StartsWith(app_id,
kPushMessagingAppIdentifierPrefix, ...));

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:479:
std::map<std::string, std::string>() /* options */,
Will this call be modified when we switch to subtypes? Do we need to branch
again in the tests when that happens?

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:631: const auto&
unregister_callback =
nit: this probably shouldn't be a reference?

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:863: void
PushMessagingServiceImpl::GetEncryptionInfoImpl(
The *Impl suffix is not really appropriate here. Maybe use
GetEncryptionInfoForAppIdentifier()?

https://codereview.chromium.org/1851423003/diff/120001/components/gcm_driver/...
File
components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1851423003/diff/120001/components/gcm_driver/...
components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:66:
if (InstanceIDWithSubtype.sSubtypeInstances.size() != 1) {
I think `protected` visibility would still work fine for this? Why does it need
to be public?

https://codereview.chromium.org/1851423003/diff/120001/content/browser/push_m...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/content/browser/push_m...
content/browser/push_messaging/push_messaging_message_filter.cc:742: const
std::vector<std::string>& push_subscription_id_and_sender_info,
nit: subscription_info?

[johnme, 2016-06-07 14:16:43]

Addressed review comments - thanks!

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_app_identifier.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:222:
DCHECK(app_id_.size() > kPrefixLength + suffix_length);
On 2016/06/02 15:53:17, Peter Beverloo - OOO June 8-16 wrote:
> nit: DCHECK_GT

Done.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:240: )));
On 2016/06/02 15:53:17, Peter Beverloo wrote:
> This is a ten line DCHECK containing significant logic, the complication of
this
> is way too high.
> 
> Let's instead separate this out in a series of variables living on the stack.
> How about the following?
> 
> ===============
>   // App identifiers created for InstanceID-based subscriptions have a suffix
>   // allowing them to be identified as such, whilst breaking the GUID syntax.
>   std::string guid = app_id_.substr(app_id_.size() - kGuidLength);
>   bool is_instance_id =
>       base::EndsWith(guid, kInstanceIDGuidSuffix,
base::CompareCase::SENSITIVE);
> 
>   if (is_instance_id) {
>     DCHECK(!base::IsValidGUID(guid));
> 
>     // Replace the suffix with valid GUID characters in order to be able to
>     // validate the rest of the string.
>     guid = guid.replace(guid.size() - kInstanceIDGuidSuffix,
>                         kInstanceIDGuidSuffix, kInstanceIDGuidSuffix, 'C');
>   }
> 
>   DCHECK(base::IsValidGUID(guid));
> ===============
> 
> That's incredibly much easier to read.
> 
> If you're worried about compiling that code in non-debug builds, let's stick
the
> entire body of this method in an "#if DCHECK_IS_ON()" block.

Done.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:338: // Create a
non-InstanceID GCM registration. We have to directly access
On 2016/06/02 15:53:17, Peter Beverloo wrote:
> micro nit: no 'we' in comments. I also question the value of the final part of
> this comment- the code only cares about the current (and maybe future) state
of
> itself.

Removed we. The 2nd part seems useful in explaining why this code is here rather
than in say PushMessagingServiceImpl.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:385: std::string*
out_token) {
On 2016/06/02 15:53:17, Peter Beverloo wrote:
> This file has an inconsistency between requiring out_* values and having them
be
> optional. Could we do one or the other? (My preference is always requiring
> them.)

Done. Made them all optional, since there are 21 calls to SubscribeSuccessfully
alone which don't pass an out_token, and it would add a lot of clutter to
require that.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:392: size_t
min_token_length = 5;
On 2016/06/02 15:53:17, Peter Beverloo wrote:
> Why 5? Why not >0? If there is no known size, let's not care about it at all?

Done (now only check it's non-empty).

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1186: // Push
subscriptions used to be non-InstanceID GCM registrations. We still need
On 2016/06/02 15:53:17, Peter Beverloo wrote:
> nit: no 'we'

Done.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1188:
IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, LegacyUnsubscribeSuccess) {
On 2016/06/02 15:53:17, Peter Beverloo - OOO June 8-16 wrote:
> Can we test receiving messages for `legacy` subscriptions?

Done

> In line with the comments I shared when reviewing the GCM Driver changes, I
> still have a very strong preference for avoiding the use of "legacy" in our
> code.

I still have a strong preference for testing migrated data that our users
continue to depend on :)

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_service_factory.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_factory.cc:31: "InstanceID
is unexpectedly disabled";
On 2016/06/02 15:53:17, Peter Beverloo - OOO June 8-16 wrote:
> s/unexpectedly//, these things don't happen by accident :)

Done.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_factory.cc:31: "InstanceID
is unexpectedly disabled";
On 2016/06/02 15:53:17, Peter Beverloo - OOO June 8-16 wrote:
> Is there a watchlist or something we can subscribe to to make sure that we
> immediately get informed when someone flips the kill-switch?

Yes: https://goto.google.com/actkg

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:196: return
app_id.rfind(kPushMessagingAppIdentifierPrefix, 0) == 0;
On 2016/06/02 15:53:17, Peter Beverloo - OOO June 8-16 wrote:
> Why can't we do a prefix search? (I.e. base::StartsWith(app_id,
> kPushMessagingAppIdentifierPrefix, ...));

Done (this was already doing a prefix search; technically StartsWith is 3 lines
[including include] instead of 1, and constructs an extra StringPiece object,
but I agree it's more readable; also it makes sense to allow case insensitive
starts with so that the DCHECKs in PushMessagingAppIdentifier get a chance to
fire).

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:479:
std::map<std::string, std::string>() /* options */,
On 2016/06/02 15:53:18, Peter Beverloo - OOO June 8-16 wrote:
> Will this call be modified when we switch to subtypes? Do we need to branch
> again in the tests when that happens?

The GetInstanceID call will be modified then. Assuming we land both InstanceID
and desktop subtypes in the same Chrome build, we don't need to fork the tests
(the tests that use InstanceID will just start using subtypes).

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:631: const auto&
unregister_callback =
On 2016/06/02 15:53:18, Peter Beverloo - OOO June 8-16 wrote:
> nit: this probably shouldn't be a reference?

Done (though technically it should be equivalent)

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:863: void
PushMessagingServiceImpl::GetEncryptionInfoImpl(
On 2016/06/02 15:53:18, Peter Beverloo - OOO June 8-16 wrote:
> The *Impl suffix is not really appropriate here. Maybe use
> GetEncryptionInfoForAppIdentifier()?

Done (GetEncryptionInfoForAppId, since it's not an identifier object)

https://codereview.chromium.org/1851423003/diff/120001/components/gcm_driver/...
File
components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1851423003/diff/120001/components/gcm_driver/...
components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:66:
if (InstanceIDWithSubtype.sSubtypeInstances.size() != 1) {
On 2016/06/02 15:53:18, Peter Beverloo - OOO June 8-16 wrote:
> I think `protected` visibility would still work fine for this? Why does it
need
> to be public?

org.chromium.chrome.browser.push_messaging.PushMessagingTest is neither a
subclass nor same package.

https://codereview.chromium.org/1851423003/diff/120001/content/browser/push_m...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/content/browser/push_m...
content/browser/push_messaging/push_messaging_message_filter.cc:742: const
std::vector<std::string>& push_subscription_id_and_sender_info,
On 2016/06/02 15:53:18, Peter Beverloo - OOO June 8-16 wrote:
> nit: subscription_info?

I like that you can tell exactly what e.g.
push_subscription_id_and_sender_info[1] means, and think it's worth the extra
verbosity in this case.

[johnme, 2016-06-07 14:16:43]

Addressed review comments - thanks!

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_app_identifier.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:222:
DCHECK(app_id_.size() > kPrefixLength + suffix_length);
On 2016/06/02 15:53:17, Peter Beverloo - OOO June 8-16 wrote:
> nit: DCHECK_GT

Done.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_app_identifier.cc:240: )));
On 2016/06/02 15:53:17, Peter Beverloo wrote:
> This is a ten line DCHECK containing significant logic, the complication of
this
> is way too high.
> 
> Let's instead separate this out in a series of variables living on the stack.
> How about the following?
> 
> ===============
>   // App identifiers created for InstanceID-based subscriptions have a suffix
>   // allowing them to be identified as such, whilst breaking the GUID syntax.
>   std::string guid = app_id_.substr(app_id_.size() - kGuidLength);
>   bool is_instance_id =
>       base::EndsWith(guid, kInstanceIDGuidSuffix,
base::CompareCase::SENSITIVE);
> 
>   if (is_instance_id) {
>     DCHECK(!base::IsValidGUID(guid));
> 
>     // Replace the suffix with valid GUID characters in order to be able to
>     // validate the rest of the string.
>     guid = guid.replace(guid.size() - kInstanceIDGuidSuffix,
>                         kInstanceIDGuidSuffix, kInstanceIDGuidSuffix, 'C');
>   }
> 
>   DCHECK(base::IsValidGUID(guid));
> ===============
> 
> That's incredibly much easier to read.
> 
> If you're worried about compiling that code in non-debug builds, let's stick
the
> entire body of this method in an "#if DCHECK_IS_ON()" block.

Done.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:338: // Create a
non-InstanceID GCM registration. We have to directly access
On 2016/06/02 15:53:17, Peter Beverloo wrote:
> micro nit: no 'we' in comments. I also question the value of the final part of
> this comment- the code only cares about the current (and maybe future) state
of
> itself.

Removed we. The 2nd part seems useful in explaining why this code is here rather
than in say PushMessagingServiceImpl.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:385: std::string*
out_token) {
On 2016/06/02 15:53:17, Peter Beverloo wrote:
> This file has an inconsistency between requiring out_* values and having them
be
> optional. Could we do one or the other? (My preference is always requiring
> them.)

Done. Made them all optional, since there are 21 calls to SubscribeSuccessfully
alone which don't pass an out_token, and it would add a lot of clutter to
require that.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:392: size_t
min_token_length = 5;
On 2016/06/02 15:53:17, Peter Beverloo wrote:
> Why 5? Why not >0? If there is no known size, let's not care about it at all?

Done (now only check it's non-empty).

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1186: // Push
subscriptions used to be non-InstanceID GCM registrations. We still need
On 2016/06/02 15:53:17, Peter Beverloo wrote:
> nit: no 'we'

Done.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1188:
IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, LegacyUnsubscribeSuccess) {
On 2016/06/02 15:53:17, Peter Beverloo - OOO June 8-16 wrote:
> Can we test receiving messages for `legacy` subscriptions?

Done

> In line with the comments I shared when reviewing the GCM Driver changes, I
> still have a very strong preference for avoiding the use of "legacy" in our
> code.

I still have a strong preference for testing migrated data that our users
continue to depend on :)

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_service_factory.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_factory.cc:31: "InstanceID
is unexpectedly disabled";
On 2016/06/02 15:53:17, Peter Beverloo - OOO June 8-16 wrote:
> s/unexpectedly//, these things don't happen by accident :)

Done.

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_factory.cc:31: "InstanceID
is unexpectedly disabled";
On 2016/06/02 15:53:17, Peter Beverloo - OOO June 8-16 wrote:
> Is there a watchlist or something we can subscribe to to make sure that we
> immediately get informed when someone flips the kill-switch?

Yes: https://goto.google.com/actkg

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:196: return
app_id.rfind(kPushMessagingAppIdentifierPrefix, 0) == 0;
On 2016/06/02 15:53:17, Peter Beverloo - OOO June 8-16 wrote:
> Why can't we do a prefix search? (I.e. base::StartsWith(app_id,
> kPushMessagingAppIdentifierPrefix, ...));

Done (this was already doing a prefix search; technically StartsWith is 3 lines
[including include] instead of 1, and constructs an extra StringPiece object,
but I agree it's more readable; also it makes sense to allow case insensitive
starts with so that the DCHECKs in PushMessagingAppIdentifier get a chance to
fire).

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:479:
std::map<std::string, std::string>() /* options */,
On 2016/06/02 15:53:18, Peter Beverloo - OOO June 8-16 wrote:
> Will this call be modified when we switch to subtypes? Do we need to branch
> again in the tests when that happens?

The GetInstanceID call will be modified then. Assuming we land both InstanceID
and desktop subtypes in the same Chrome build, we don't need to fork the tests
(the tests that use InstanceID will just start using subtypes).

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:631: const auto&
unregister_callback =
On 2016/06/02 15:53:18, Peter Beverloo - OOO June 8-16 wrote:
> nit: this probably shouldn't be a reference?

Done (though technically it should be equivalent)

https://codereview.chromium.org/1851423003/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:863: void
PushMessagingServiceImpl::GetEncryptionInfoImpl(
On 2016/06/02 15:53:18, Peter Beverloo - OOO June 8-16 wrote:
> The *Impl suffix is not really appropriate here. Maybe use
> GetEncryptionInfoForAppIdentifier()?

Done (GetEncryptionInfoForAppId, since it's not an identifier object)

https://codereview.chromium.org/1851423003/diff/120001/components/gcm_driver/...
File
components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1851423003/diff/120001/components/gcm_driver/...
components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:66:
if (InstanceIDWithSubtype.sSubtypeInstances.size() != 1) {
On 2016/06/02 15:53:18, Peter Beverloo - OOO June 8-16 wrote:
> I think `protected` visibility would still work fine for this? Why does it
need
> to be public?

org.chromium.chrome.browser.push_messaging.PushMessagingTest is neither a
subclass nor same package.

https://codereview.chromium.org/1851423003/diff/120001/content/browser/push_m...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1851423003/diff/120001/content/browser/push_m...
content/browser/push_messaging/push_messaging_message_filter.cc:742: const
std::vector<std::string>& push_subscription_id_and_sender_info,
On 2016/06/02 15:53:18, Peter Beverloo - OOO June 8-16 wrote:
> nit: subscription_info?

I like that you can tell exactly what e.g.
push_subscription_id_and_sender_info[1] means, and think it's worth the extra
verbosity in this case.

[Peter Beverloo, 2016-06-08 12:33:06]

lgtm

[johnme, 2016-07-01 13:55:05]

Description was changed from

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch

Also depends on
- https://codereview.chromium.org/1945753002 transactional SW userdata
- https://codereview.chromium.org/1944863002 fix sender ID storage

BUG=589461
==========

to

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. https://codereview.chromium.org/2111973002 adds IID subtypes on desktop
9. this patch

Also depends on
- https://codereview.chromium.org/1945753002 transactional SW userdata
- https://codereview.chromium.org/1944863002 fix sender ID storage

BUG=589461, 533498
==========

[johnme, 2016-08-24 18:13:13]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 18:13:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2016-08-24 18:19:35]

johnme@chromium.org changed reviewers:
+ avi@chromium.org, dfalcantara@chromium.org, finnur@chromium.org

[johnme, 2016-08-24 18:19:36]

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

dfalcantara@chromium.org: Please review changes in chrome/android/BUILD.gn and
chrome/android/chrome_apk.gyp

finnur@chromium.org: Please review changes in chrome/browser/extensions/ (it's
no longer possible for the test to read those two properties in
service_worker_apitest.cc, but this is already extensively covered by other
tests - they were only being checked here due to copy/paste)

[commit-bot: I haz the power, 2016-08-24 18:30:27]

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

[commit-bot: I haz the power, 2016-08-24 18:30:29]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gone, 2016-08-24 18:48:10]

https://chromiumcodereview.appspot.com/1851423003/diff/180001/chrome/android/...
File chrome/android/chrome_apk.gyp (right):

https://chromiumcodereview.appspot.com/1851423003/diff/180001/chrome/android/...
chrome/android/chrome_apk.gyp:281:
'../../components/components.gyp:instance_id_driver_test_support_java',
1) Are we still maintaining GYP files?
2) Any reason why this doesn't include instance_id_driver_java like above?  Is
it transiently included?

[johnme, 2016-08-24 19:17:01]

https://chromiumcodereview.appspot.com/1851423003/diff/180001/chrome/android/...
File chrome/android/chrome_apk.gyp (right):

https://chromiumcodereview.appspot.com/1851423003/diff/180001/chrome/android/...
chrome/android/chrome_apk.gyp:281:
'../../components/components.gyp:instance_id_driver_test_support_java',
On 2016/08/24 18:48:10, dfalcantara wrote:
> 1) Are we still maintaining GYP files?

I guess not
(https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NZkPr-CXvQ0/dis...),
but since I've already updated it, it seems it may as well stay?

> 2) Any reason why this doesn't include instance_id_driver_java like above?  Is
> it transiently included?

It's not a public dependency in GN, whereas in GYP IIRC all dependencies were
public.

[johnme, 2016-08-24 19:17:01]

https://chromiumcodereview.appspot.com/1851423003/diff/180001/chrome/android/...
File chrome/android/chrome_apk.gyp (right):

https://chromiumcodereview.appspot.com/1851423003/diff/180001/chrome/android/...
chrome/android/chrome_apk.gyp:281:
'../../components/components.gyp:instance_id_driver_test_support_java',
On 2016/08/24 18:48:10, dfalcantara wrote:
> 1) Are we still maintaining GYP files?

I guess not
(https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NZkPr-CXvQ0/dis...),
but since I've already updated it, it seems it may as well stay?

> 2) Any reason why this doesn't include instance_id_driver_java like above?  Is
> it transiently included?

It's not a public dependency in GN, whereas in GYP IIRC all dependencies were
public.

[gone, 2016-08-24 19:18:05]

Cool, lgtm.

[johnme, 2016-08-24 19:47:45]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 19:48:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2016-08-24 21:08:43]

johnme@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org
- finnur@chromium.org

[johnme, 2016-08-24 21:08:44]

-finnur +rdevlin.cronin, who's probably a more appropriate reviewer for these
parts of chrome/browser/extensions

[Devlin, 2016-08-24 21:34:58]

My knowledge here is pretty rough, so might be asking an unnecessary question.

https://codereview.chromium.org/1851423003/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (left):

https://codereview.chromium.org/1851423003/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:766: EXPECT_EQ("1234567890",
gcm_service()->last_registered_sender_ids()[0]);
Why are we taking this out?

[johnme, 2016-08-24 22:03:44]

Addressed Devlin's review comment - thanks!

https://codereview.chromium.org/1851423003/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (left):

https://codereview.chromium.org/1851423003/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:766: EXPECT_EQ("1234567890",
gcm_service()->last_registered_sender_ids()[0]);
On 2016/08/24 21:34:58, Devlin wrote:
> Why are we taking this out?

Those debug accessors do not support Instance ID. However I've managed to write
equivalent checks after all, by mirroring the changes to
push_messaging_browsertest.cc (which ServiceWorkerPushMessagingTest here is
based off).

[johnme, 2016-08-24 22:03:58]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 22:05:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2016-08-24 22:08:46]

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

[johnme, 2016-08-24 22:08:47]

-avi (seems to be out) +piman

[piman, 2016-08-24 22:09:08]

content/ LGTM

[Devlin, 2016-08-24 23:25:34]

extensions lgtm

[commit-bot: I haz the power, 2016-08-25 01:23:57]

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

[commit-bot: I haz the power, 2016-08-25 01:23:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[johnme, 2016-09-06 16:34:55]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-09-06 16:34:55]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
dfalcantara@chromium.org, rdevlin.cronin@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1851423003/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-06 16:35:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 16:38:18]

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

[commit-bot: I haz the power, 2016-09-06 16:38:20]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[johnme, 2016-09-06 16:41:56]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-09-06 16:41:56]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
rdevlin.cronin@chromium.org, dfalcantara@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1851423003/#ps260001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-06 16:42:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 19:38:13]

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

[commit-bot: I haz the power, 2016-09-06 19:38:15]

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

[johnme, 2016-09-08 11:57:01]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-09-08 11:57:01]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
rdevlin.cronin@chromium.org, dfalcantara@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1851423003/#ps280001
(title: "Fix checkdeps")

[commit-bot: I haz the power, 2016-09-08 11:57:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 12:29:36]

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

[commit-bot: I haz the power, 2016-09-08 12:29:38]

Exceeded global retry quota

[johnme, 2016-09-08 13:57:26]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-09-08 13:57:27]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
rdevlin.cronin@chromium.org, dfalcantara@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1851423003/#ps300001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-08 13:57:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 17:23:45]

Description was changed from

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. https://codereview.chromium.org/2111973002 adds IID subtypes on desktop
9. this patch

Also depends on
- https://codereview.chromium.org/1945753002 transactional SW userdata
- https://codereview.chromium.org/1944863002 fix sender ID storage

BUG=589461, 533498
==========

to

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. https://codereview.chromium.org/2111973002 adds IID subtypes on desktop
9. this patch

Also depends on
- https://codereview.chromium.org/1945753002 transactional SW userdata
- https://codereview.chromium.org/1944863002 fix sender ID storage

BUG=589461, 533498
==========

[commit-bot: I haz the power, 2016-09-08 17:23:48]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-09-08 17:25:54]

Description was changed from

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. https://codereview.chromium.org/2111973002 adds IID subtypes on desktop
9. this patch

Also depends on
- https://codereview.chromium.org/1945753002 transactional SW userdata
- https://codereview.chromium.org/1944863002 fix sender ID storage

BUG=589461, 533498
==========

to

==========
Make Web Push use InstanceID tokens instead of GCM registrations

On both desktop and Android, the Push API will now be backed by
InstanceID tokens instead of GCM registrations.

This should have no compatibility impact, since GCM was already
silently converting Push API registrations to InstanceID format
server-side.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. https://codereview.chromium.org/2111973002 adds IID subtypes on desktop
9. this patch

Also depends on
- https://codereview.chromium.org/1945753002 transactional SW userdata
- https://codereview.chromium.org/1944863002 fix sender ID storage

BUG=589461, 533498

Committed: https://crrev.com/a504573264e3021119337fdfbebc76c7f72a3510
Cr-Commit-Position: refs/heads/master@{#417323}
==========

[commit-bot: I haz the power, 2016-09-08 17:25:56]

Patchset 16 (id:??) landed as
https://crrev.com/a504573264e3021119337fdfbebc76c7f72a3510
Cr-Commit-Position: refs/heads/master@{#417323}