Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1851423003:
Make Web Push use InstanceID tokens instead of GCM registrations (Closed)
|
Created:
4 years, 8 months ago by johnme Modified:
4 years, 3 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, sadrul, mlamouri+watch-test-runner_chromium.org, zea+watch_chromium.org, mlamouri+watch-content_chromium.org, Peter Beverloo, johnme+watch_chromium.org, jam, mvanouwerkerk+watch_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, mkwst+moarreviews-shell_chromium.org, harkness+watch_chromium.org, jochen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@iid4default Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake 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}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebase #Patch Set 4 : Fix GN/GYP #
Total comments: 14
Patch Set 5 : Address review comments, and rewrite following rebase (e.g. no longer depends on NestedMessagePumpA… #
Total comments: 32
Patch Set 6 : Major update: hooked up crypto, added non-IID unsubscribe test, added UseInstanceID, addressed comments, rebased #Patch Set 7 : Rebase (main conflics in browsertest and PMMF) #
Total comments: 30
Patch Set 8 : Address 3rd round review comments #Patch Set 9 : Rebase and use subtypes on desktop #Patch Set 10 : Rebase #
Total comments: 2
Patch Set 11 : Rebase (remove force_subtype_for_app_id) #
Total comments: 2
Patch Set 12 : Add back checks to service_worker_apitest.cc #Patch Set 13 : Rebase #Patch Set 14 : Rebase #Patch Set 15 : Fix checkdeps #Patch Set 16 : Rebase #Dependent Patchsets: Messages
Total messages: 64 (35 generated)
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 ==========
[email protected] changed reviewers: + [email protected]
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.
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 ==========
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.
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..
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 ==========
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 ==========
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 ==========
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 ==========
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 ==========
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_*
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?
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.
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.
lgtm
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 ==========
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
[email protected] changed reviewers: + [email protected], [email protected], [email protected]
[email protected]: Please review changes in content/public/browser/push_messaging_service.* [email protected]: Please review changes in chrome/android/BUILD.gn and chrome/android/chrome_apk.gyp [email protected]: 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)
The CQ bit was unchecked by [email protected]
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_...)
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?
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.
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.
Cool, lgtm.
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
[email protected] changed reviewers: + [email protected] - [email protected]
-finnur +rdevlin.cronin, who's probably a more appropriate reviewer for these parts of chrome/browser/extensions
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?
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).
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
[email protected] changed reviewers: + [email protected] - [email protected]
-avi (seems to be out) +piman
content/ LGTM
extensions lgtm
The CQ bit was unchecked by [email protected]
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_...)
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected], [email protected], [email protected], [email protected] Link to the patchset: https://codereview.chromium.org/1851423003/#ps240001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
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...)
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected], [email protected], [email protected], [email protected] Link to the patchset: https://codereview.chromium.org/1851423003/#ps260001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
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_...)
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected], [email protected], [email protected], [email protected] Link to the patchset: https://codereview.chromium.org/1851423003/#ps280001 (title: "Fix checkdeps")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Exceeded global retry quota
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected], [email protected], [email protected], [email protected] Link to the patchset: https://codereview.chromium.org/1851423003/#ps300001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/a504573264e3021119337fdfbebc76c7f72a3510 Cr-Commit-Position: refs/heads/master@{#417323} |
