Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2469293002:
Handle push resubscribes with no sender info (avoids DCHECK) (Closed)
DescriptionHandle push resubscribes with no sender info (avoids DCHECK)
- Prior to this patch you would fail a DCHECK if you called
PushManager.subscribe() with a sender id or key and then
PushManager.subscribe() without a sender id or key.
- This fixes that and only allows resubscribing with no sender info
when the original subscribe was made with a gcm_sender_id.
Otherwise we throw an AbortError - no key provided.
BUG=659230
Committed: https://crrev.com/d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816
Cr-Commit-Position: refs/heads/master@{#430601}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Review comments, fixup SubscribeWorker test #
Total comments: 7
Patch Set 3 : Move logic to DidGetSenderIdFromStorage #Patch Set 4 : Remove redundant bool from DidGetSenderId and tidy it up a bit #
Total comments: 6
Patch Set 5 : Comments from patch set 4 #Patch Set 6 : rebase #
Total comments: 47
Patch Set 7 : Addressing johnme@'s comments on push_messaging_message_filter code #Patch Set 8 : Split the code path for registered/unregistered as in patch set 2; comments #
Total comments: 6
Patch Set 9 : responding to more comments #
Total comments: 1
Patch Set 10 : call it fixed_sender_id when it is (hopefully) fixed #
Dependent Patchsets: Messages
Total messages: 47 (21 generated)
The CQ bit was checked by awdf@chromium.org 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...
awdf@chromium.org changed reviewers: + harkness@chromium.org
+harkness for review
Forgot to say, you might wanna check out the doc linked on the ticket if the intention is unclear ( https://docs.google.com/document/d/1_ghioYLVi4cnEC-RswpkLHJTyTKaLv3IODulCEZ3C...) - but hopefully my code comments are good enough On Wed, 2 Nov 2016 at 13:52 <awdf@chromium.org> wrote: > Reviewers: harkness > CL: https://codereview.chromium.org/2469293002/ > > Message: > +harkness for review > > Description: > Handle push resubscribes with no sender info (avoids DCHECK) > > - Prior to this patch you would fail a DCHECK if you called > PushManager.subscribe() with a sender id or key and then > PushManager.subscribe() without a sender id or key. > > - This fixes that and only allows resubscribing with no sender info > when the original subscribe was made with a gcm_sender_id. > Otherwise we throw an AbortError - no key provided. > > BUG=659230 > > Affected files (+273, -18 lines): > M chrome/browser/push_messaging/push_messaging_browsertest.cc > M chrome/test/data/push_messaging/push_test.js > M chrome/test/data/push_messaging/service_worker.js > A + chrome/test/data/push_messaging/test_no_manifest.html > M content/browser/push_messaging/push_messaging_message_filter.cc > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_message_filter.cc:386: // Reject subscribes with no sender info, unless the stored id is numeric, Hm, actually not sure I need any of this stuff. It depends if my new rules for when you're allowed to not provide a key should just apply to resubscribes (subscribe(), subscribe()) or also apply if you do subscribe(), unsubscribe(), subscribe(). (Right now they apply in both causing a test failure for the latter situation from PMBrowserTest.Subscribeworker)..
https://codereview.chromium.org/2469293002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/2469293002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_browsertest.cc:677: // This should also fail as the original key was not a gcm_sender_id. Maybe call it a numeric string, since we're not really checking for anything else. On like 718 as well. https://codereview.chromium.org/2469293002/diff/1/chrome/test/data/push_messa... File chrome/test/data/push_messaging/push_test.js (right): https://codereview.chromium.org/2469293002/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/push_test.js:122: function documentSubscribePushWithNumber() { nit NumericKey ? https://codereview.chromium.org/2469293002/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/push_test.js:141: // succeed if the worker was previously subscribed and fail otherwise. Can you update this comment please to include info about the previous format of the key. https://codereview.chromium.org/2469293002/diff/1/chrome/test/data/push_messa... File chrome/test/data/push_messaging/service_worker.js (right): https://codereview.chromium.org/2469293002/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/service_worker.js:54: } else if (event.data.command == 'workerSubscribePushWithNumber') { nit NumericKey? https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_message_filter.cc:294: const std::vector<std::string>& push_registration_id_and_sender_id, As we discussed in person, I think the code would be a lot clearer if you only do the no registration check here, and move all the sender id checking into DidGetSenderIdFromStorage. https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_message_filter.cc:304: auto callback = base::Bind( Unless I'm overlooking something, you can refactor this to make it much simpler if you move the callback definition and the PostTask to after the if/else and just have the if set a local to data.options.sender_info or stored_sender_id. const auto& sender_id = push_registration_id_and_sender_id; if (...) { sender_id = data.options.sender_info; } else { // check things } callback = ... BrowserThread::PostTask(... sender_id ...); return;
I am still working on the further refactor to move logic to DidGetSenderIdFromStorage - I think it's probably a good idea but I'll put it in a separate patch so it can be easily compared side by side so we can see if it does make things easier to reason about. https://codereview.chromium.org/2469293002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/2469293002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_browsertest.cc:677: // This should also fail as the original key was not a gcm_sender_id. On 2016/11/02 18:48:12, harkness wrote: > Maybe call it a numeric string, since we're not really checking for anything > else. On like 718 as well. Done. https://codereview.chromium.org/2469293002/diff/1/chrome/test/data/push_messa... File chrome/test/data/push_messaging/push_test.js (right): https://codereview.chromium.org/2469293002/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/push_test.js:122: function documentSubscribePushWithNumber() { On 2016/11/02 18:48:12, harkness wrote: > nit NumericKey ? Done. https://codereview.chromium.org/2469293002/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/push_test.js:141: // succeed if the worker was previously subscribed and fail otherwise. On 2016/11/02 18:48:12, harkness wrote: > Can you update this comment please to include info about the previous format of > the key. Done. https://codereview.chromium.org/2469293002/diff/1/chrome/test/data/push_messa... File chrome/test/data/push_messaging/service_worker.js (right): https://codereview.chromium.org/2469293002/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/service_worker.js:54: } else if (event.data.command == 'workerSubscribePushWithNumber') { On 2016/11/02 18:48:12, harkness wrote: > nit NumericKey? Oops, this actually isn't called, (since it's one of those 'only support for simplicity' cases, I didn't write explicit tests for it in the end). Removed. https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_message_filter.cc:294: const std::vector<std::string>& push_registration_id_and_sender_id, On 2016/11/02 18:48:12, harkness wrote: > As we discussed in person, I think the code would be a lot clearer if you only > do the no registration check here, and move all the sender id checking into > DidGetSenderIdFromStorage. Ok yeah I think you're right and it would look simpler, it just means I'll have to pass an extra bool to DidGetSenderIdFromStorage -shouldRegisterNewSubscription?- so it knows whether to register a new subscription or just check encryption info + return success as it's doing here when one already exists. https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_message_filter.cc:304: auto callback = base::Bind( On 2016/11/02 18:48:12, harkness wrote: > Unless I'm overlooking something, you can refactor this to make it much simpler > if you move the callback definition and the PostTask to after the if/else and > just have the if set a local to data.options.sender_info or stored_sender_id. > > const auto& sender_id = push_registration_id_and_sender_id; > if (...) { > sender_id = data.options.sender_info; > } else { > // check things > } > > callback = ... > BrowserThread::PostTask(... sender_id ...); > return; > Done. Agree it's much nicer not to repeat the identical callback and near-identical postTask. https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_message_filter.cc:386: // Reject subscribes with no sender info, unless the stored id is numeric, On 2016/11/02 15:03:00, awdf wrote: > Hm, actually not sure I need any of this stuff. It depends if my new rules for > when you're allowed to not provide a key should just apply to resubscribes > (subscribe(), subscribe()) or also apply if you do subscribe(), unsubscribe(), > subscribe(). (Right now they apply in both causing a test failure for the latter > situation from PMBrowserTest.Subscribeworker).. (Leaving it in and fixing up that test instead as it makes sense to apply the rules consistently).
https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_message_filter.cc:294: const std::vector<std::string>& push_registration_id_and_sender_id, On 2016/11/03 14:10:11, awdf wrote: > On 2016/11/02 18:48:12, harkness wrote: > > As we discussed in person, I think the code would be a lot clearer if you only > > do the no registration check here, and move all the sender id checking into > > DidGetSenderIdFromStorage. > > Ok yeah I think you're right and it would look simpler, it just means I'll have > to pass an extra bool to DidGetSenderIdFromStorage > -shouldRegisterNewSubscription?- so it knows whether to register a new > subscription or just check encryption info + return success as it's doing here > when one already exists. And also push_registration_id needs to be passed through for this case but not the other. Hm I could pass it as a nullable string and check for its existence instead of having a separate bool to say whether to register, what do you think?
Moved the logic to the other method in the latest patch. Which involved changing its signature a bit. What do you think?
https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_message_filter.cc:294: const std::vector<std::string>& push_registration_id_and_sender_id, On 2016/11/03 14:21:21, awdf wrote: > On 2016/11/03 14:10:11, awdf wrote: > > On 2016/11/02 18:48:12, harkness wrote: > > > As we discussed in person, I think the code would be a lot clearer if you > only > > > do the no registration check here, and move all the sender id checking into > > > DidGetSenderIdFromStorage. > > > > Ok yeah I think you're right and it would look simpler, it just means I'll > have > > to pass an extra bool to DidGetSenderIdFromStorage > > -shouldRegisterNewSubscription?- so it knows whether to register a new > > subscription or just check encryption info + return success as it's doing here > > when one already exists. > > And also push_registration_id needs to be passed through for this case but not > the other. Hm I could pass it as a nullable string and check for its existence > instead of having a separate bool to say whether to register, what do you think? Yeah, I like the nullable string. It's already a string coming from the GetRegistrationUserData call. https://codereview.chromium.org/2469293002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_message_filter.cc:386: // Reject subscribes with no sender info, unless the stored id is numeric, On 2016/11/03 14:10:11, awdf wrote: > On 2016/11/02 15:03:00, awdf wrote: > > Hm, actually not sure I need any of this stuff. It depends if my new rules for > > when you're allowed to not provide a key should just apply to resubscribes > > (subscribe(), subscribe()) or also apply if you do subscribe(), unsubscribe(), > > subscribe(). (Right now they apply in both causing a test failure for the > latter > > situation from PMBrowserTest.Subscribeworker).. > > (Leaving it in and fixing up that test instead as it makes sense to apply the > rules consistently). I think it's ok to leave this in for now, but long term we want to get down to a single place this is in the code, which will be addressed by the refactor from line 294. https://codereview.chromium.org/2469293002/diff/20001/content/browser/push_me... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_message_filter.cc:300: const auto& stored_sender_id = push_registration_id_and_sender_id[1]; Just call this sender_id, since on line 307 you're going to assign it to that anyway. https://codereview.chromium.org/2469293002/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_message_filter.cc:301: bool stored_sender_id_is_numeric = No need to have this, just embed the calculation in the if statement. It's not so long that it will be unreadable, and that way it will only execute if data.options.sender_info is empty. https://codereview.chromium.org/2469293002/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_message_filter.cc:372: if (data.options.sender_info.empty() && !stored_sender_id_is_numeric) { Again, avoid the local, just embed the calculation.
https://codereview.chromium.org/2469293002/diff/20001/content/browser/push_me... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_message_filter.cc:300: const auto& stored_sender_id = push_registration_id_and_sender_id[1]; On 2016/11/03 15:09:28, harkness wrote: > Just call this sender_id, since on line 307 you're going to assign it to that > anyway. Done. https://codereview.chromium.org/2469293002/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_message_filter.cc:301: bool stored_sender_id_is_numeric = On 2016/11/03 15:09:27, harkness wrote: > No need to have this, just embed the calculation in the if statement. It's not > so long that it will be unreadable, and that way it will only execute if > data.options.sender_info is empty. I was going for readability over performance here, is ContainsOnlyChars that expensive? (And I was hoping maybe the compiler would optimize it into a shortcircuiting statement anyway, but that might be wishful thinking..)
The CQ bit was checked by awdf@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2469293002/diff/20001/content/browser/push_me... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_message_filter.cc:301: bool stored_sender_id_is_numeric = On 2016/11/03 15:44:13, awdf wrote: > On 2016/11/03 15:09:27, harkness wrote: > > No need to have this, just embed the calculation in the if statement. It's not > > so long that it will be unreadable, and that way it will only execute if > > data.options.sender_info is empty. > > I was going for readability over performance here, is ContainsOnlyChars that > expensive? (And I was hoping maybe the compiler would optimize it into a > shortcircuiting statement anyway, but that might be wishful thinking..) The compiler will definitely remove it. Personally I agree that it is more readable to have the local, but my experience with chrome has been that readability locals are discouraged. I'll let peter be the final say on this one. https://codereview.chromium.org/2469293002/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_message_filter.cc:372: if (data.options.sender_info.empty() && !stored_sender_id_is_numeric) { On 2016/11/03 15:09:27, harkness wrote: > Again, avoid the local, just embed the calculation. Acknowledged. https://codereview.chromium.org/2469293002/diff/60001/content/browser/push_me... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/60001/content/browser/push_me... content/browser/push_messaging/push_messaging_message_filter.cc:316: // There is no existing registration and the sender_info passed in was I think you can get rid of this if you check for the Register case from line 313 first. https://codereview.chromium.org/2469293002/diff/60001/content/browser/push_me... content/browser/push_messaging/push_messaging_message_filter.cc:363: } Just make 364 an "else if" and you can remove the return on 362 and it's a bit clearer to reason about. https://codereview.chromium.org/2469293002/diff/60001/content/browser/push_me... content/browser/push_messaging/push_messaging_message_filter.cc:365: RegisterData mutated_data = data; Can you add comments similar to line 356, explaining how we get into this situation? They don't have to be super long, just something to distinguish the cases. Also on 372.
https://codereview.chromium.org/2469293002/diff/60001/content/browser/push_me... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/60001/content/browser/push_me... content/browser/push_messaging/push_messaging_message_filter.cc:316: // There is no existing registration and the sender_info passed in was On 2016/11/03 17:05:22, harkness wrote: > I think you can get rid of this if you check for the Register case from line 313 > first. Done. https://codereview.chromium.org/2469293002/diff/60001/content/browser/push_me... content/browser/push_messaging/push_messaging_message_filter.cc:363: } On 2016/11/03 17:05:22, harkness wrote: > Just make 364 an "else if" and you can remove the return on 362 and it's a bit > clearer to reason about. Done. https://codereview.chromium.org/2469293002/diff/60001/content/browser/push_me... content/browser/push_messaging/push_messaging_message_filter.cc:365: RegisterData mutated_data = data; On 2016/11/03 17:05:22, harkness wrote: > Can you add comments similar to line 356, explaining how we get into this > situation? They don't have to be super long, just something to distinguish the > cases. > > Also on 372. Done.
lgtm
The CQ bit was checked by awdf@chromium.org 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...
awdf@chromium.org changed reviewers: + johnme@chromium.org
+johnme@ for OWNERS review
Initial comments (haven't yet looked at tests) https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:283: data.requesting_origin = service_worker_registration->pattern().GetOrigin(); I think you want to add: if (data.options.sender_info.empty() && data.FromDocument()) { SendSubscriptionError(data, PUSH_REGISTRATION_STATUS_NO_SENDER_ID); return; } below this line. Otherwise you're allowing entries in the top row of the your table ("Resubscribe from doc"). https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:302: if (service_worker_status != SERVICE_WORKER_OK && (This patch feels a little awkward, because when there was a stored existing push registration ID and data.options.sender_info was not empty, DidGetSenderIdFromStorage doesn't currently do anything with the sender ID fetched from storage. But I get that it'll make more sense once https://codereview.chromium.org/2436393002 lands on top, so that's fine.) https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:309: if (service_worker_status == SERVICE_WORKER_OK) { Nit: it'd be a little clearer to do both at the same time: std::string stored_push_registration_id; if (service_worker_status == SERVICE_WORKER_OK) { DCHECK_EQ(1u, push_registration_id.size()); stored_push_registration_id = push_registration_id[0]; } https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:322: void PushMessagingMessageFilter::DidGetEncryptionKeys( Nit: Please move this method below DidGetSenderIdFromStorage (in both .h and .cc) to reflect the new order in which they are called. https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:339: void PushMessagingMessageFilter::DidGetSenderIdFromStorage( This method got a little harder to understand, since it now has two roles. How about adding a comment to the top along the lines of "The stored sender ID was fetched: - because sender_info was omitted, so consider using the stored sender - and/or because there was a pre-existing registration, so sender_info should match the stored sender" and splitting the code up with nested ifs and returns: if (data.options.sender_info.empty()) { if (!stored_sender_id_is_numeric) { SendSubscriptionError(... return; } if (push_registration_id.empty()) { PostTask(RegisterOnUI... return; } } DCHECK(!push_registration_id.empty()); // TODO(awdf): Check... PostTask(GetEncryptionInfoOnUI... wdyt? https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:350: bool stored_sender_id_is_numeric = Nit: s/is_numeric/is_gcm_project_number/ https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:353: // Reject subscribes with no sender info, unless the stored id is numeric, I know this comment is already quite long, but could you add "even if there is a stored push_registration_id" after the first comma? https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:362: mutated_data.options.sender_info = sender_id[0]; Please add a DCHECK(data.options.sender_info.empty()) to document the fact that it's known to already be blank in this case. Unless you do the nested ifs thing I suggest above, in which case this'll be unnecessary. https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:369: // TODO(crbug.com/638924): Check that stored sender ID equals Nit: chromium style says to put the username that knows about the issue in the TODO brackets (so one of us). The bug number can be part of the TODO's description instead.
let me know if this is good enough or you want something more similar to patch 2 and can tell me how i can avoid duplicating the is_numeric check in that case! https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:283: data.requesting_origin = service_worker_registration->pattern().GetOrigin(); On 2016/11/04 14:46:12, johnme wrote: > I think you want to add: > > if (data.options.sender_info.empty() && data.FromDocument()) { > SendSubscriptionError(data, PUSH_REGISTRATION_STATUS_NO_SENDER_ID); > return; > } > > below this line. Otherwise you're allowing entries in the top row of the your > table ("Resubscribe from doc"). Actually we should never get here in the top-row case, because it's caught earlier at https://cs.chromium.org/chromium/src/content/renderer/push_messaging/push_mes... - as confirmed by my new test for that situation passing (ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest). I believe I have added test coverage for all the green/red cells in the doc, but let me know if you think I've missed something.. https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:302: if (service_worker_status != SERVICE_WORKER_OK && On 2016/11/04 14:46:12, johnme wrote: > (This patch feels a little awkward, because when there was a stored existing > push registration ID and data.options.sender_info was not empty, > DidGetSenderIdFromStorage doesn't currently do anything with the sender ID > fetched from storage. But I get that it'll make more sense once > https://codereview.chromium.org/2436393002 lands on top, so that's fine.) Acknowledged. https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:309: if (service_worker_status == SERVICE_WORKER_OK) { On 2016/11/04 14:46:12, johnme wrote: > Nit: it'd be a little clearer to do both at the same time: > > std::string stored_push_registration_id; > if (service_worker_status == SERVICE_WORKER_OK) { > DCHECK_EQ(1u, push_registration_id.size()); > stored_push_registration_id = push_registration_id[0]; > } Done. https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:322: void PushMessagingMessageFilter::DidGetEncryptionKeys( On 2016/11/04 14:46:12, johnme wrote: > Nit: Please move this method below DidGetSenderIdFromStorage (in both .h and > .cc) to reflect the new order in which they are called. Done. https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:339: void PushMessagingMessageFilter::DidGetSenderIdFromStorage( On 2016/11/04 14:46:12, johnme wrote: > This method got a little harder to understand, since it now has two roles. How > about adding a comment to the top along the lines of > > "The stored sender ID was fetched: > - because sender_info was omitted, so consider using the stored sender > - and/or because there was a pre-existing registration, so sender_info should > match the stored sender" > > and splitting the code up with nested ifs and returns: > > > if (data.options.sender_info.empty()) { > if (!stored_sender_id_is_numeric) { > SendSubscriptionError(... > return; > } > if (push_registration_id.empty()) { > PostTask(RegisterOnUI... > return; > } > } > DCHECK(!push_registration_id.empty()); > // TODO(awdf): Check... > PostTask(GetEncryptionInfoOnUI... > > > wdyt? Agree it could use a comment explaining why we're here, but I've phrased it slightly differently and re-arranged the code to match. Wdyt? https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:350: bool stored_sender_id_is_numeric = On 2016/11/04 14:46:12, johnme wrote: > Nit: s/is_numeric/is_gcm_project_number/ Done. https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:353: // Reject subscribes with no sender info, unless the stored id is numeric, On 2016/11/04 14:46:12, johnme wrote: > I know this comment is already quite long, but could you add "even if there is a > stored push_registration_id" after the first comma? Done. https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:362: mutated_data.options.sender_info = sender_id[0]; On 2016/11/04 14:46:12, johnme wrote: > Please add a DCHECK(data.options.sender_info.empty()) to document the fact that > it's known to already be blank in this case. Unless you do the nested ifs thing > I suggest above, in which case this'll be unnecessary. Done. https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:369: // TODO(crbug.com/638924): Check that stored sender ID equals On 2016/11/04 14:46:12, johnme wrote: > Nit: chromium style says to put the username that knows about the issue in the > TODO brackets (so one of us). The bug number can be part of the TODO's > description instead. Done.
On 2016/11/04 16:21:36, awdf wrote:
> let me know if this is good enough or you want something more similar to patch
2
> and can tell me how i can avoid duplicating the is_numeric check in that case!
Sorry, I think it was actually clearer in patch set 2 :-(
You could avoid duplicating the is_numeric etc logic with a helper method:
std::string FixSenderInfo(const std::string& sender_info, const std::string&
stored_sender_id) {
if (!sender_info.empty())
return sender_info;
if (base::ContainsOnlyChars(stored_sender_id, "0123456789"))
return stored_sender_id;
// Explanatory comment.
return std::string();
}
Then you'd only need to duplicate a call to it like the following:
std::string sender_id = FixSenderInfo(data.options.sender_info,
stored_sender_id);
if (sender_id.empty()){
SendSubscriptionError(data, PUSH_REGISTRATION_STATUS_NO_SENDER_ID);
return;
}
Tests look good. I left a few comments (on patch set 6, which was current when I started reviewing them). https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:534: // Now run the subscribe from the service worker with a key. Nit: I know you didn't write this sentence, but it sounds like it's referring to some different service worker that has a key associated. Mind fixing that whilst you're editing this line? https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:546: SubscribeWorkerUsingManifest) { I've suggested adding an extra unsubscribe and resubscribe to the tests you added, then you can kill SubscribeWorkerUsingManifest since it'll be a strict subset of ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:593: ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest) { Nit: These test names could probably be shortened a little (and WithoutKeyFailure below confusingly sounds like it doesn't fail). How about ResubscribeAfterDocumentManifest, ResubscribeAfterWorkerNumericASK, etc? https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:631: EXPECT_EQ(token1, token2); Now unsubscribe then subscribe successfully from a worker without key? https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:670: script_result); Now unsubscribe then subscribe successfully from a worker without key? https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:711: script_result); Now unsubscribe then subscribe successfully from a worker without key? https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:745: script_result); Now subscribe successfully from a worker without key, unsubscribe, and subscribe successfully from a worker without key again? https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:770: RunScript("documentSubscribePushWithNumericKey()", &script_result)); s/document/worker/, based on test name https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:779: script_result); Now subscribe successfully from a worker without key, unsubscribe, and subscribe successfully from a worker without key again? https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:283: data.requesting_origin = service_worker_registration->pattern().GetOrigin(); On 2016/11/04 16:21:36, awdf wrote: > On 2016/11/04 14:46:12, johnme wrote: > > I think you want to add: > > > > if (data.options.sender_info.empty() && data.FromDocument()) { > > SendSubscriptionError(data, PUSH_REGISTRATION_STATUS_NO_SENDER_ID); > > return; > > } > > > > below this line. Otherwise you're allowing entries in the top row of the your > > table ("Resubscribe from doc"). > > Actually we should never get here in the top-row case, because it's caught > earlier at > https://cs.chromium.org/chromium/src/content/renderer/push_messaging/push_mes... > - as confirmed by my new test for that situation passing > (ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest). I believe I have added > test coverage for all the green/red cells in the doc, but let me know if you > think I've missed something.. Huh, I wouldn't have thought to look in PushMessagingDispatcher for that. Mind adding a DCHECK for that anyway?
On 2016/11/04 16:48:19, johnme wrote:
> On 2016/11/04 16:21:36, awdf wrote:
> > let me know if this is good enough or you want something more similar to
patch
> 2
> > and can tell me how i can avoid duplicating the is_numeric check in that
case!
>
> Sorry, I think it was actually clearer in patch set 2 :-(
>
> You could avoid duplicating the is_numeric etc logic with a helper method:
>
> std::string FixSenderInfo(const std::string& sender_info, const std::string&
> stored_sender_id) {
> if (!sender_info.empty())
> return sender_info;
> if (base::ContainsOnlyChars(stored_sender_id, "0123456789"))
> return stored_sender_id;
> // Explanatory comment.
> return std::string();
> }
>
> Then you'd only need to duplicate a call to it like the following:
>
> std::string sender_id = FixSenderInfo(data.options.sender_info,
> stored_sender_id);
> if (sender_id.empty()){
> SendSubscriptionError(data, PUSH_REGISTRATION_STATUS_NO_SENDER_ID);
> return;
> }
ok but in the next patch where I check for mismatched ids I'm gonna have to
check the fixed sender id against the stored sender id again if it's nonempty..
might have to end up passing a status back from FixSenderInfo if we want it to
trigger multiple errors, and have the fixed sender info as an output parameter.
But I'll leave that till the next patch.
On 2016/11/07 12:24:59, awdf wrote:
> On 2016/11/04 16:48:19, johnme wrote:
> > On 2016/11/04 16:21:36, awdf wrote:
> > > let me know if this is good enough or you want something more similar to
> patch
> > 2
> > > and can tell me how i can avoid duplicating the is_numeric check in that
> case!
> >
> > Sorry, I think it was actually clearer in patch set 2 :-(
> >
> > You could avoid duplicating the is_numeric etc logic with a helper method:
> >
> > std::string FixSenderInfo(const std::string& sender_info, const std::string&
> > stored_sender_id) {
> > if (!sender_info.empty())
> > return sender_info;
> > if (base::ContainsOnlyChars(stored_sender_id, "0123456789"))
> > return stored_sender_id;
> > // Explanatory comment.
> > return std::string();
> > }
> >
> > Then you'd only need to duplicate a call to it like the following:
> >
> > std::string sender_id = FixSenderInfo(data.options.sender_info,
> > stored_sender_id);
> > if (sender_id.empty()){
> > SendSubscriptionError(data, PUSH_REGISTRATION_STATUS_NO_SENDER_ID);
> > return;
> > }
>
> ok but in the next patch where I check for mismatched ids I'm gonna have to
> check the fixed sender id against the stored sender id again if it's
nonempty..
> might have to end up passing a status back from FixSenderInfo if we want it to
> trigger multiple errors, and have the fixed sender info as an output
parameter.
> But I'll leave that till the next patch.
Technically you only need to check for a mismatch in the
DidCheckForExistingRegistration SERVICE_WORKER_OK case. In
PushMessagingMessageFilter::DidGetSenderIdFromStorage (as of patch set 2),
data.options.sender_info is always empty (and actually, it would help to add a
DCHECK documenting that). So you could just add the mismatch check in
DidCheckForExistingRegistration instead of in FixSenderInfo, though I'd be fine
with adding it FixSenderInfo instead if you prefer to keep all the logic
together. If so then either returning a status (and moving fixed_sender_id to an
out param) or having FixSenderInfo directly send the errors (and the caller
would just return if the result is empty) both sound reasonable. Whatever you
find cleanest :)
Patchset #8 (id:140001) has been deleted
Patchset #8 (id:160001) has been deleted
https://codereview.chromium.org/2469293002/diff/1/chrome/test/data/push_messa... File chrome/test/data/push_messaging/service_worker.js (right): https://codereview.chromium.org/2469293002/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/service_worker.js:54: } else if (event.data.command == 'workerSubscribePushWithNumber') { On 2016/11/03 14:10:11, awdf wrote: > On 2016/11/02 18:48:12, harkness wrote: > > nit NumericKey? > > Oops, this actually isn't called, (since it's one of those 'only support for > simplicity' cases, I didn't write explicit tests for it in the end). Removed. Reinstated. Discussing with John we figured it's not such a bad idea to have explicit tests for those yellow cases after all, in order to notice if something changes, so long as there's a comment saying it's not ideal behaviour. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:534: // Now run the subscribe from the service worker with a key. On 2016/11/04 17:43:36, johnme wrote: > Nit: I know you didn't write this sentence, but it sounds like it's referring to > some different service worker that has a key associated. Mind fixing that whilst > you're editing this line? Done. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:546: SubscribeWorkerUsingManifest) { On 2016/11/04 17:43:36, johnme wrote: > I've suggested adding an extra unsubscribe and resubscribe to the tests you > added, then you can kill SubscribeWorkerUsingManifest since it'll be a strict > subset of ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest. Done. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:593: ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest) { On 2016/11/04 17:43:37, johnme wrote: > Nit: These test names could probably be shortened a little (and > WithoutKeyFailure below confusingly sounds like it doesn't fail). How about > ResubscribeAfterDocumentManifest, ResubscribeAfterWorkerNumericASK, etc? Very good point that WithoutKeyFailure can be parsed as "without key-failure" rather than "..without key. Failure!" I didn't think of that.. Aside: I actually wanted to call them something like "ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest_shouldFail" but was trying to fit in with the naming style already in place which doesn't really give directives or verbs (blah_shouldFail / blahFailsWhen..), instead it seems to mostly use nouns (blahFailure). I prefer to err on the side of clarity rather than brevity in test names (since they never need to be typed out in code), so have stuck with the long names but swapped the order of 'WithoutKey' and 'Failure'. I still think it *would* be clearer if it could be 'ResubscribeWithoutKeyAfter...ShouldFail' - wdyt? https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:631: EXPECT_EQ(token1, token2); On 2016/11/04 17:43:37, johnme wrote: > Now unsubscribe then subscribe successfully from a worker without key? Done. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:670: script_result); On 2016/11/04 17:43:37, johnme wrote: > Now unsubscribe then subscribe successfully from a worker without key? I don't think that would be successful actually, didn't we say we'd also reject this case in the ticket? https://bugs.chromium.org/p/chromium/issues/detail?id=659230&desc=4#c15 (unless you're implying 'reload with manifest & resubscribe from document then subscribe from worker' too ?) Maybe I should explicitly check it fails too though.. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:711: script_result); On 2016/11/04 17:43:36, johnme wrote: > Now unsubscribe then subscribe successfully from a worker without key? Same here, shouldn't it fail again? https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:745: script_result); On 2016/11/04 17:43:37, johnme wrote: > Now subscribe successfully from a worker without key, unsubscribe, and subscribe > successfully from a worker without key again? and here.. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:770: RunScript("documentSubscribePushWithNumericKey()", &script_result)); On 2016/11/04 17:43:37, johnme wrote: > s/document/worker/, based on test name Done. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:779: script_result); On 2016/11/04 17:43:37, johnme wrote: > Now subscribe successfully from a worker without key, unsubscribe, and subscribe > successfully from a worker without key again? .. https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:283: data.requesting_origin = service_worker_registration->pattern().GetOrigin(); On 2016/11/04 17:43:37, johnme wrote: > On 2016/11/04 16:21:36, awdf wrote: > > On 2016/11/04 14:46:12, johnme wrote: > > > I think you want to add: > > > > > > if (data.options.sender_info.empty() && data.FromDocument()) { > > > SendSubscriptionError(data, PUSH_REGISTRATION_STATUS_NO_SENDER_ID); > > > return; > > > } > > > > > > below this line. Otherwise you're allowing entries in the top row of the > your > > > table ("Resubscribe from doc"). > > > > Actually we should never get here in the top-row case, because it's caught > > earlier at > > > https://cs.chromium.org/chromium/src/content/renderer/push_messaging/push_mes... > > - as confirmed by my new test for that situation passing > > (ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest). I believe I have > added > > test coverage for all the green/red cells in the doc, but let me know if you > > think I've missed something.. > > Huh, I wouldn't have thought to look in PushMessagingDispatcher for that. Mind > adding a DCHECK for that anyway? Done.
Code looks good, thanks! A few replies and nits about the tests. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:593: ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest) { On 2016/11/07 16:54:30, awdf wrote: > On 2016/11/04 17:43:37, johnme wrote: > > Nit: These test names could probably be shortened a little (and > > WithoutKeyFailure below confusingly sounds like it doesn't fail). How about > > ResubscribeAfterDocumentManifest, ResubscribeAfterWorkerNumericASK, etc? > > Very good point that WithoutKeyFailure can be parsed as "without key-failure" > rather than "..without key. Failure!" I didn't think of that.. > > Aside: I actually wanted to call them something like > "ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest_shouldFail" but was > trying to fit in with the naming style already in place which doesn't really > give directives or verbs (blah_shouldFail / blahFailsWhen..), instead it seems > to mostly use nouns (blahFailure). > > I prefer to err on the side of clarity rather than brevity in test names (since > they never need to be typed out in code), so have stuck with the long names but > swapped the order of 'WithoutKey' and 'Failure'. I still think it *would* be > clearer if it could be 'ResubscribeWithoutKeyAfter...ShouldFail' - wdyt? Since each of these tests is testing three things (that subscribing without key from document always fails, that subscribing from sw either fails or succeeds, and that subscribing from sw after unsubscribing does the same thing), I think it'd be clearer to omit Failure/ShouldFail from the test names, and let people read the ASSERT/EXPECT statements (and associated comments). But I don't mind the current names if you still prefer them. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:670: script_result); On 2016/11/07 16:54:30, awdf wrote: > On 2016/11/04 17:43:37, johnme wrote: > > Now unsubscribe then subscribe successfully from a worker without key? > > I don't think that would be successful actually, didn't we say we'd also reject > this case in the ticket? > > https://bugs.chromium.org/p/chromium/issues/detail?id=659230&desc=4#c15 > > (unless you're implying 'reload with manifest & resubscribe from document then > subscribe from worker' too ?) > > Maybe I should explicitly check it fails too though.. Sorry yes, I mean unsubscribe then check that it still fails. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:711: script_result); On 2016/11/07 16:54:30, awdf wrote: > On 2016/11/04 17:43:36, johnme wrote: > > Now unsubscribe then subscribe successfully from a worker without key? > > Same here, shouldn't it fail again? Yup, I also meant unsubscribe then check that it still fails. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:745: script_result); On 2016/11/07 16:54:30, awdf wrote: > On 2016/11/04 17:43:37, johnme wrote: > > Now subscribe successfully from a worker without key, unsubscribe, and > subscribe > > successfully from a worker without key again? > > and here.. No, these do indeed succeed. And I see that you check that they succeed in patch set 8's ResubscribeFailureWithoutKeyAfterSubscribingFromDocumentWithNumber - thanks :) https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:779: script_result); On 2016/11/07 16:54:30, awdf wrote: > On 2016/11/04 17:43:37, johnme wrote: > > Now subscribe successfully from a worker without key, unsubscribe, and > subscribe > > successfully from a worker without key again? > > .. Ditto I see that you check they succeed :) https://codereview.chromium.org/2469293002/diff/180001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/2469293002/diff/180001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:594: EndpointToToken(script_result, false /* standard_protocol */, &token2)); Nit: seems more consistent to call this token3 (ditto several times below), even though technically we know that token1==token2. https://codereview.chromium.org/2469293002/diff/180001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:697: // Run the subscribe from the service worker with a key. s/key/numeric key/ or similar https://codereview.chromium.org/2469293002/diff/180001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:756: // Run the subscribe from the service worker with a key. s/key/numeric key/ or similar
https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:593: ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest) { On 2016/11/07 18:11:59, johnme wrote: > On 2016/11/07 16:54:30, awdf wrote: > > On 2016/11/04 17:43:37, johnme wrote: > > > Nit: These test names could probably be shortened a little (and > > > WithoutKeyFailure below confusingly sounds like it doesn't fail). How about > > > ResubscribeAfterDocumentManifest, ResubscribeAfterWorkerNumericASK, etc? > > > > Very good point that WithoutKeyFailure can be parsed as "without key-failure" > > rather than "..without key. Failure!" I didn't think of that.. > > > > Aside: I actually wanted to call them something like > > "ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest_shouldFail" but was > > trying to fit in with the naming style already in place which doesn't really > > give directives or verbs (blah_shouldFail / blahFailsWhen..), instead it seems > > to mostly use nouns (blahFailure). > > > > I prefer to err on the side of clarity rather than brevity in test names > (since > > they never need to be typed out in code), so have stuck with the long names > but > > swapped the order of 'WithoutKey' and 'Failure'. I still think it *would* be > > clearer if it could be 'ResubscribeWithoutKeyAfter...ShouldFail' - wdyt? > > Since each of these tests is testing three things (that subscribing without key > from document always fails, that subscribing from sw either fails or succeeds, > and that subscribing from sw after unsubscribing does the same thing), I think > it'd be clearer to omit Failure/ShouldFail from the test names, and let people > read the ASSERT/EXPECT statements (and associated comments). But I don't mind > the current names if you still prefer them. Done. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:670: script_result); On 2016/11/07 18:11:59, johnme wrote: > On 2016/11/07 16:54:30, awdf wrote: > > On 2016/11/04 17:43:37, johnme wrote: > > > Now unsubscribe then subscribe successfully from a worker without key? > > > > I don't think that would be successful actually, didn't we say we'd also > reject > > this case in the ticket? > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=659230&desc=4#c15 > > > > (unless you're implying 'reload with manifest & resubscribe from document then > > subscribe from worker' too ?) > > > > Maybe I should explicitly check it fails too though.. > > Sorry yes, I mean unsubscribe then check that it still fails. Done. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:711: script_result); On 2016/11/07 18:11:59, johnme wrote: > On 2016/11/07 16:54:30, awdf wrote: > > On 2016/11/04 17:43:36, johnme wrote: > > > Now unsubscribe then subscribe successfully from a worker without key? > > > > Same here, shouldn't it fail again? > > Yup, I also meant unsubscribe then check that it still fails. Done. https://codereview.chromium.org/2469293002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:745: script_result); On 2016/11/07 18:11:59, johnme wrote: > On 2016/11/07 16:54:30, awdf wrote: > > On 2016/11/04 17:43:37, johnme wrote: > > > Now subscribe successfully from a worker without key, unsubscribe, and > > subscribe > > > successfully from a worker without key again? > > > > and here.. > > No, these do indeed succeed. And I see that you check that they succeed in patch > set 8's ResubscribeFailureWithoutKeyAfterSubscribingFromDocumentWithNumber - > thanks :) Acknowledged. https://codereview.chromium.org/2469293002/diff/180001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/2469293002/diff/180001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:594: EndpointToToken(script_result, false /* standard_protocol */, &token2)); On 2016/11/07 18:11:59, johnme wrote: > Nit: seems more consistent to call this token3 (ditto several times below), even > though technically we know that token1==token2. Done. https://codereview.chromium.org/2469293002/diff/180001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:697: // Run the subscribe from the service worker with a key. On 2016/11/07 18:11:59, johnme wrote: > s/key/numeric key/ or similar Done. It should also say document not service worker - fixed. https://codereview.chromium.org/2469293002/diff/180001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:756: // Run the subscribe from the service worker with a key. On 2016/11/07 18:11:59, johnme wrote: > s/key/numeric key/ or similar Done.
The CQ bit was checked by awdf@chromium.org 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...
lgtm, looks great, thanks for your patience! :) https://codereview.chromium.org/2469293002/diff/200001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2469293002/diff/200001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:327: // TODO(crbug.com/638924): Check that stored sender ID equals Nit: TODO(username) and bug number in comment instead.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by awdf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from harkness@chromium.org, johnme@chromium.org Link to the patchset: https://codereview.chromium.org/2469293002/#ps220001 (title: "call it fixed_sender_id when it is (hopefully) fixed")
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.
Committed patchset #10 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Handle push resubscribes with no sender info (avoids DCHECK) - Prior to this patch you would fail a DCHECK if you called PushManager.subscribe() with a sender id or key and then PushManager.subscribe() without a sender id or key. - This fixes that and only allows resubscribing with no sender info when the original subscribe was made with a gcm_sender_id. Otherwise we throw an AbortError - no key provided. BUG=659230 ========== to ========== Handle push resubscribes with no sender info (avoids DCHECK) - Prior to this patch you would fail a DCHECK if you called PushManager.subscribe() with a sender id or key and then PushManager.subscribe() without a sender id or key. - This fixes that and only allows resubscribing with no sender info when the original subscribe was made with a gcm_sender_id. Otherwise we throw an AbortError - no key provided. BUG=659230 Committed: https://crrev.com/d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816 Cr-Commit-Position: refs/heads/master@{#430601} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816 Cr-Commit-Position: refs/heads/master@{#430601} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
