Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10915216:
Browser test for the push messaging feature (Closed)
|
Created:
8 years, 3 months ago by Pete Williamson Modified:
8 years, 3 months ago CC:
chromium-reviews, ncarter (slow), mihaip-chromium-reviews_chromium.org, akalin, Raghu Simha, Aaron Boodman, haitaol1, tim (not reviewing) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionBrowser test for the push messaging feature
We've implemented a push messaging feature and checked it in with a unit test.
This change is adding a browser test to get better coverage in a running
environment.
The browser test basically brings up a small packaged app, and injects a message
into the sync code. For the test to succeed, the message must arrive into the
JS code of the packaged app.
BUG=148550
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157296
Patch Set 1 #Patch Set 2 : refactor to reduce duplication #
Total comments: 19
Patch Set 3 : CR fixes per DCheng and MihaiP #
Total comments: 2
Patch Set 4 : Comment CR fixes per MihaiP #Patch Set 5 : CR changes per DCheng #
Total comments: 2
Patch Set 6 : CR changes per DCheng #Patch Set 7 : Change name of obj_id variable #
Total comments: 5
Patch Set 8 : CR changes per Akalin #
Total comments: 2
Patch Set 9 : CR change per Alakin #
Messages
Total messages: 20 (0 generated)
Ben: Please review chrome_tests.gypi only as OWNER (small change) Fred: Please review the changes in sync as OWNER Mihai: Please do owners review in the extension directory as OWNER Daniel: Please review everything
In general, this feels very similar to the existing test at http://git.chromium.org/gitweb/?p=chromium.git;a=blob;f=chrome/browser/extens.... It looks like the main difference is that we inject the callback at a different layer (PSS instead of PushMessagingInvalidationHandler). Given that, would it make sense to update the api test to use the PSS test injection instead?
(removed Ben since we are no longer changing chrome_tests.gypi) CR changes per DCheng - moved the test into our existing file, elimitated duplication of the test extension. Ready for re-review
https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/browser/extensi... File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc (right): https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:68: // test that a push introduced into the sync code makes it to the extension Nit: capitalize sentence, end it with a period. (applies to other comments too). https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/test/data/exten... File chrome/test/data/extensions/api_test/push_messaging/event_dispatch.js (right): https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/test/data/exten... chrome/test/data/extensions/api_test/push_messaging/event_dispatch.js:6: // comment these back in for debugging Please don't commit with commented-out code (especially because the code is incorrect, the variable is called details, not message). The assertEq calls will print the actual value if the comparison fails, so I don't think you need the log statements in any case.
http://codereview.chromium.org/10915216/diff/9/chrome/browser/extensions/api/... File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc (right): http://codereview.chromium.org/10915216/diff/9/chrome/browser/extensions/api/... chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:75: // launch the test app In general, comments like this aren't needed. Ditto to comments on lines 83, 93, and 96. http://codereview.chromium.org/10915216/diff/9/chrome/browser/extensions/api/... chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:86: ASSERT_TRUE(NULL != pss); ASSERT_TRUE(pss) to match style in other parts of this tests and other extension api tests. http://codereview.chromium.org/10915216/diff/9/chrome/browser/sync/profile_sy... File chrome/browser/sync/profile_sync_service.cc (right): http://codereview.chromium.org/10915216/diff/9/chrome/browser/sync/profile_sy... chrome/browser/sync/profile_sync_service.cc:82: using syncer::ObjectIdSet; If you want to add using lines for this, you should remove redundant syncer:: prefixes from the other places in this file that refer to these types. Otherwise, I would just remove these using lines and use syncer:: for consistency. http://codereview.chromium.org/10915216/diff/9/chrome/browser/sync/profile_sy... chrome/browser/sync/profile_sync_service.cc:479: const std::string& id_name, Why not have this take an ObjectId or an ObjectIdSet as an argument? Then we can just pass it directly to OnIncomingInvalidation() after we call ObjectIdSetToStateMap(ids, payload) http://codereview.chromium.org/10915216/diff/9/chrome/browser/sync/profile_sy... File chrome/browser/sync/profile_sync_service.h (right): http://codereview.chromium.org/10915216/diff/9/chrome/browser/sync/profile_sy... chrome/browser/sync/profile_sync_service.h:578: // test method for system test to allow a test to send in an invalidation I think you could simplify this comment to "Method to trigger an invalidation in tests." http://codereview.chromium.org/10915216/diff/9/chrome/browser/sync/profile_sy... chrome/browser/sync/profile_sync_service.h:579: virtual void SendNotificationForTest( Suggestion: this should be called SendInvalidationForTest(). http://codereview.chromium.org/10915216/diff/9/chrome/browser/sync/profile_sy... chrome/browser/sync/profile_sync_service.h:579: virtual void SendNotificationForTest( Doesn't need to be virtual. It's possible that this method should be added to InvalidationFrontend as a virtual method though, and then overridden here.
New patch uploaded to address CR comments. https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/browser/extensi... File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc (right): https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:68: // test that a push introduced into the sync code makes it to the extension On 2012/09/13 20:37:09, Mihai Parparita wrote: > Nit: capitalize sentence, end it with a period. > > (applies to other comments too). Done. https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:75: // launch the test app On 2012/09/13 21:00:48, dcheng wrote: > In general, comments like this aren't needed. Ditto to comments on lines 83, 93, > and 96. Done. https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/browser/extensi... chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:86: ASSERT_TRUE(NULL != pss); On 2012/09/13 21:00:48, dcheng wrote: > ASSERT_TRUE(pss) to match style in other parts of this tests and other extension > api tests. Done. https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service.cc (right): https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:82: using syncer::ObjectIdSet; On 2012/09/13 21:00:48, dcheng wrote: > If you want to add using lines for this, you should remove redundant syncer:: > prefixes from the other places in this file that refer to these types. > Otherwise, I would just remove these using lines and use syncer:: for > consistency. Done. https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:479: const std::string& id_name, On 2012/09/13 21:00:48, dcheng wrote: > Why not have this take an ObjectId or an ObjectIdSet as an argument? Then we can > just pass it directly to OnIncomingInvalidation() after we call > ObjectIdSetToStateMap(ids, payload) Since we have to look up the name in the invalidator registrar, it seems to me that we have better encapsulation if we leave the logic to do the name lookup here, instead of trying to do this from the test code. Also, as best I can tell, invalidator_registrar_ is not exposed to clients of the ProfileSyncService class, so the test code would have no way to walk the list of ObjectIds looking for the right one. https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service.h (right): https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.h:578: // test method for system test to allow a test to send in an invalidation On 2012/09/13 21:00:48, dcheng wrote: > I think you could simplify this comment to "Method to trigger an invalidation in > tests." Done. https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.h:579: virtual void SendNotificationForTest( On 2012/09/13 21:00:48, dcheng wrote: > Suggestion: this should be called SendInvalidationForTest(). Done. https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.h:579: virtual void SendNotificationForTest( On 2012/09/13 21:00:48, dcheng wrote: > Doesn't need to be virtual. It's possible that this method should be added to > InvalidationFrontend as a virtual method though, and then overridden here. Made non virtual, but I didn't want to extend the encapsulation break any further than I needed to. https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/test/data/exten... File chrome/test/data/extensions/api_test/push_messaging/event_dispatch.js (right): https://chromiumcodereview.appspot.com/10915216/diff/9/chrome/test/data/exten... chrome/test/data/extensions/api_test/push_messaging/event_dispatch.js:6: // comment these back in for debugging On 2012/09/13 20:37:09, Mihai Parparita wrote: > Please don't commit with commented-out code (especially because the code is > incorrect, the variable is called details, not message). > > The assertEq calls will print the actual value if the comparison fails, so I > don't think you need the log statements in any case. Done.
LGTM https://chromiumcodereview.appspot.com/10915216/diff/5002/chrome/browser/sync... File chrome/browser/sync/profile_sync_service.cc (right): https://chromiumcodereview.appspot.com/10915216/diff/5002/chrome/browser/sync... chrome/browser/sync/profile_sync_service.cc:475: // test method for system test to allow a test to send in an invalidation More comments that need to be capitalized and end with a period.
http://codereview.chromium.org/10915216/diff/9/chrome/browser/sync/profile_sy... File chrome/browser/sync/profile_sync_service.cc (right): http://codereview.chromium.org/10915216/diff/9/chrome/browser/sync/profile_sy... chrome/browser/sync/profile_sync_service.cc:479: const std::string& id_name, On 2012/09/14 19:14:48, Pete Williamson wrote: > On 2012/09/13 21:00:48, dcheng wrote: > > Why not have this take an ObjectId or an ObjectIdSet as an argument? Then we > can > > just pass it directly to OnIncomingInvalidation() after we call > > ObjectIdSetToStateMap(ids, payload) > > Since we have to look up the name in the invalidator registrar, it seems to me > that we have better encapsulation if we leave the logic to do the name lookup > here, instead of trying to do this from the test code. Also, as best I can > tell, invalidator_registrar_ is not exposed to clients of the ProfileSyncService > class, so the test code would have no way to walk the list of ObjectIds looking > for the right one. I don't quite understand what you mean. You don't need to use InvalidationRegistrar outside do you? You just pass in a constructed object ID with the correct source ID and name. OnIncomingInvalidation has to do the lookup either way, so this just results in a double lookup, essentially. http://codereview.chromium.org/10915216/diff/5002/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc (right): http://codereview.chromium.org/10915216/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:82: ProfileSyncService* pss = ProfileSyncServiceFactory:: Nit: break the line after the ( instead and keep the two sides of :: together instead?
Added new changes for DCheng Ah, I see, I can use the source to build the ID, I was thinking it was a key I had to look up. Fixed now. I could also refactor the kSourceId into a header file if you like, but since it is temporary, I decided to leave it in the .cc file, duplicated with the other one, and we can remove both once the constant gets pushed.
http://codereview.chromium.org/10915216/diff/6002/chrome/browser/extensions/a... File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc (right): http://codereview.chromium.org/10915216/diff/6002/chrome/browser/extensions/a... chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:95: invalidation::ObjectId objId(kSourceId, id); See next comment about naming. http://codereview.chromium.org/10915216/diff/6002/chrome/browser/sync/profile... File chrome/browser/sync/profile_sync_service.h (right): http://codereview.chromium.org/10915216/diff/6002/chrome/browser/sync/profile... chrome/browser/sync/profile_sync_service.h:580: const invalidation::ObjectId objId, Pass by reference. Also use underscore_naming (and we also generally don't abbreviate names, but you could probably just call this "id").
OK, fixed as Daniel suggests, Daniel, please check the changes.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petewil@chromium.org/10915216/15002
Presubmit check for 10915216-15002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/sync
Presubmit checks took 1.1s to calculate.
few nits https://chromiumcodereview.appspot.com/10915216/diff/15002/chrome/browser/syn... File chrome/browser/sync/profile_sync_service.cc (right): https://chromiumcodereview.appspot.com/10915216/diff/15002/chrome/browser/syn... chrome/browser/sync/profile_sync_service.cc:475: // Test method for system test to allow a test to send in an invalidation. this comment doesn't say much, either. I think you can just remove it. https://chromiumcodereview.appspot.com/10915216/diff/15002/chrome/browser/syn... chrome/browser/sync/profile_sync_service.cc:479: // insert the object into the list of IDs to notify this comment doesn't say much https://chromiumcodereview.appspot.com/10915216/diff/15002/chrome/browser/syn... chrome/browser/sync/profile_sync_service.cc:483: // Build an object state map, and use it to forward the call to i don't know if this says muchmore, either https://chromiumcodereview.appspot.com/10915216/diff/15002/chrome/browser/syn... File chrome/browser/sync/profile_sync_service.h (right): https://chromiumcodereview.appspot.com/10915216/diff/15002/chrome/browser/syn... chrome/browser/sync/profile_sync_service.h:578: // Method to trigger an invalidation in tests. Comment doesn't say much. Suggest something like: // Simulate an incoming notification for the given id and payload. https://chromiumcodereview.appspot.com/10915216/diff/15002/chrome/browser/syn... chrome/browser/sync/profile_sync_service.h:579: void SendInvalidationForTest( 'Send' should be 'Emit', since you want to simulate an *incoming* notification and not an outgoing one
CR changes updated per Akalin - Fred, can you please take a look? Thanks!
lgtm http://codereview.chromium.org/10915216/diff/11007/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc (right): http://codereview.chromium.org/10915216/diff/11007/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:91: std::string id("U/"); can you just do: const std::string id = "U/" + extension->id() + "/1"; ?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petewil@chromium.org/10915216/17001
Change committed as 157296
http://codereview.chromium.org/10915216/diff/11007/chrome/browser/extensions/... File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc (right): http://codereview.chromium.org/10915216/diff/11007/chrome/browser/extensions/... chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:91: std::string id("U/"); On 2012/09/18 00:12:33, akalin wrote: > can you just do: > > const std::string id = "U/" + extension->id() + "/1"; > > ? += is generally more efficient than + since it results in less temporaries. It is possible to make + more efficient (for example, the WebKit String implementation does), but I don't think typical STL implementations try to optimize this case. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
