Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1579413004:
Move push event dispatching out of ServiceWorkerVersion. (Closed)
|
Created:
4 years, 11 months ago by Marijn Kruisselbrink Modified:
4 years, 10 months ago CC:
blink-worker-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, horo+watch_chromium.org, jam, johnme+watch_chromium.org, jsbell+serviceworker_chromium.org, kinuko+serviceworker, kinuko+watch, michaeln, mvanouwerkerk+watch_chromium.org, nhiroki, Peter Beverloo, serviceworker-reviews, tzik Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove push event dispatching out of ServiceWorkerVersion.
This also involves updating the various ServiceWorkerVersion unit
tests to not depend on actual push events.
BUG=570820
Committed: https://crrev.com/173005c1c7134c6883c2431774c64bdde84d48c6
Cr-Commit-Position: refs/heads/master@{#371395}
Patch Set 1 #
Total comments: 27
Patch Set 2 : nits #Patch Set 3 : rebase #Patch Set 4 : Have a simplified combined StartRequest/DispatchEvent/FinishRequest method #
Total comments: 2
Patch Set 5 : My preferred approach #Patch Set 6 : hopefully correct tests #
Total comments: 4
Patch Set 7 : Combine DispatchEvent and FinishRequest into DispatchSimpleEvent for "simple" events #Patch Set 8 : Fix DispatchSimpleEvent comment #
Dependent Patchsets: Messages
Total messages: 20 (7 generated)
Description was changed from ========== WIP Move push event out of SWVersion BUG= ========== to ========== Move push event dispatching out of ServiceWorkerVersion. This also involves updating the various ServiceWorkerVersion unit tests to not depend on actual push events. BUG=570820 ==========
Patchset #1 (id:1) has been deleted
mek@chromium.org changed reviewers: + johnme@chromium.org, nhiroki@chromium.org
This one is a bit more involved than the notification click event CL. In particular I hope I understood the intention of the various SWVersion unittests that used to emit push events correctly, so the new code still tests the same things.
lgtm https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version_unittest.cc:243: SERVICE_WORKER_ERROR_NETWORK; // dummy value nit: SERVICE_WORKER_ERROR_MAX_VALUE would be more suitable for a dummy value. I use it these days.
https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... File content/browser/push_messaging/push_messaging_router.cc (right): https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.cc:26: BrowserThread::PostTask( Nit: While you're touching this file, please add a DCHECK_CURRENTLY_ON(BrowserThread::IO) here :) https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.cc:31: void OnPushEventResponse(scoped_refptr<ServiceWorkerVersion> service_worker, [see comment in .h first] Please add DCHECK_CURRENTLY_ON(BrowserThread::IO). https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.cc:38: service_worker->FinishRequest(request_id); [see comment in .h first] Please apply equivalent tweaks as in https://codereview.chromium.org/1575283003, e.g. check the return value of FinishEvent. https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.cc:69: void PushMessagingRouter::DeliverMessageToWorker( [see comment in .h first] Nit: please move this between PushMessagingRouter::FindServiceWorkerRegistrationCallback and PushMessagingRouter::DeliverMessageEnd to keep them in order of execution. https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.cc:74: if (service_worker->running_status() != ServiceWorkerVersion::RUNNING) { [see comment in .h first] Nit: In https://codereview.chromium.org/1575283003/ DispatchNotificationClickEventOnWorker is always called from RunAfterStartWorker, whereas here DeliverMessageToWorker is called directly and passes itself to RunAfterStartWorker. I don't really mind which you go with, but it seems cleaner to do the same in both unless there's a fundamental difference. https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... File content/browser/push_messaging/push_messaging_router.h (right): https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.h:40: static void DeliverMessageToWorker( It's kind of messy that there are now two public methods here with similar purpose, just different levels of abstraction. Looking at the implementations it seems this and OnPushEventResponse are mainly boilerplate, and are very similar to the DispatchNotificationClickEventOnWorker and OnNotificationClickEventReply methods added by https://codereview.chromium.org/1575283003. Rather than each feature implementing similar methods, can you make DispatchEvent automatically call StartRequest, TRACE_EVENT1 and FinishRequest (taking an additional parameter for the metrics event type if necessary)? And it seems reasonable to transform blink::WebServiceWorkerEventResultRejected into SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED in ServiceWorkerVersion; that's not something that features should have to worry about. https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version_unittest.cc:258: EXPECT_TRUE(version_->FinishRequest(request_id)); The previous test was actually dispatching an event, rather than merely calling StartRequest and FinishRequest. Is this still testing the same thing? https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version_unittest.cc:813: version_->StartRequest(ServiceWorkerMetrics::EventType::PUSH, Ditto is this testing the same thing?
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... File content/browser/push_messaging/push_messaging_router.cc (right): https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.cc:26: BrowserThread::PostTask( On 2016/01/18 at 19:06:19, johnme wrote: > Nit: While you're touching this file, please add a DCHECK_CURRENTLY_ON(BrowserThread::IO) here :) Done https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.cc:31: void OnPushEventResponse(scoped_refptr<ServiceWorkerVersion> service_worker, On 2016/01/18 at 19:06:19, johnme wrote: > [see comment in .h first] > > Please add DCHECK_CURRENTLY_ON(BrowserThread::IO). Done https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.cc:38: service_worker->FinishRequest(request_id); On 2016/01/18 at 19:06:19, johnme wrote: > [see comment in .h first] > > Please apply equivalent tweaks as in https://codereview.chromium.org/1575283003, e.g. check the return value of FinishEvent. Done https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.cc:69: void PushMessagingRouter::DeliverMessageToWorker( On 2016/01/18 at 19:06:19, johnme wrote: > [see comment in .h first] > > Nit: please move this between PushMessagingRouter::FindServiceWorkerRegistrationCallback and PushMessagingRouter::DeliverMessageEnd to keep them in order of execution. That would go against the chromium coding style: "Function declaration order should match function definition order." (and I can't reorder the declaration order since all public methods should come before all private methods). https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.cc:74: if (service_worker->running_status() != ServiceWorkerVersion::RUNNING) { On 2016/01/18 at 19:06:19, johnme wrote: > [see comment in .h first] > > Nit: In https://codereview.chromium.org/1575283003/ DispatchNotificationClickEventOnWorker is always called from RunAfterStartWorker, whereas here DeliverMessageToWorker is called directly and passes itself to RunAfterStartWorker. > > I don't really mind which you go with, but it seems cleaner to do the same in both unless there's a fundamental difference. The reason for the difference was to keep the changes to ServiceWorkerInternalsUI minimal. In general I prefer the what I did in the notification click patch, but to do that here would require adding a RunAfterStartWorker call in ServiceWorkerInternalsUI (which I'm happy to add if you prefer them to be the same). https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... File content/browser/push_messaging/push_messaging_router.h (right): https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.h:40: static void DeliverMessageToWorker( On 2016/01/18 at 19:06:19, johnme wrote: > It's kind of messy that there are now two public methods here with similar purpose, just different levels of abstraction. The only reason there is a second public method is because ServiceWorkerInternalsUI needs a way to send a message to a specific worker rather than a specific registration. > Looking at the implementations it seems this and OnPushEventResponse are mainly boilerplate, and are very similar to the DispatchNotificationClickEventOnWorker and OnNotificationClickEventReply methods added by https://codereview.chromium.org/1575283003. > > Rather than each feature implementing similar methods, can you make DispatchEvent automatically call StartRequest, TRACE_EVENT1 and FinishRequest (taking an additional parameter for the metrics event type if necessary)? And it seems reasonable to transform blink::WebServiceWorkerEventResultRejected into SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED in ServiceWorkerVersion; that's not something that features should have to worry about. I could add some kind of convenience wrapper, yes... I'm not sure if it's worth it though. StartRequest, FinishRequest, and possibly even DispatchEvent would still have to all remain (Start and Finish for mojo based events, and DispatchEvent because a spec compliant fetch implementation would require possibly multiple replies to the same event; one for respondWith, and a separate once the last waitUntil has been resolved). Of course a convenience wrapper would slightly simplify the code for existing events like push and notificationclick, but this won't effect new code at all, since it all should be using mojo anyway (from what I heard the tentative/aggressive plan is to completely replace IPC with mojo by the end of the year...). Having a convenience wrapper for the deprecated code-path instead of keeping the code closer to what it would look like in a mojo world seems a bit odd as well. In this case the extra layer of indirection (at least for push) is because I tried to keep the API exposed to service_worker_internals_ui unchanged. The code here could be a little bit simpler by making bigger changes to ServiceWorkerInternalsUI. And I'm not even sure how your suggestion to automatically transfer IPC parameters of one type to another type in the passed callback would work? I guess this convenience wrapper would only work if the response IPC has two parameters, a int request_id and a blink::WebServiceWorkerEventResult? That would already rule out using it for the notificationclick event, since that event currently doesn't have a event result passed in its IPC (I guess template trickery could distinguish based on the arguments of the callback, but that complexity really doesn't seem worth it). Partly I think why the current code seems weird is because it is doing weird things. Why is the TRACE_EVENT1 only done if the event was dispatched more or less succesfully? Why is it traced at all? Wouldn't it make more sense to do an async trace from StartRequest till FinishRequest rather than for whatever reason tracing how long handling a succesfull event takes? I think it might make a lot of sense to remove the TRACE_EVENT parts of all the various features and instead have TRACE_EVENT_ASYNC_BEGIN/END pairs in StartRequest/FinishRequest, but rather than change behavior during these refactorings, I'd rather first complete the refactoring without changing behavior and then make improvements. So I think while adding a convenience wrapper has some benefits, I don't think it's overall worthwhile to do: - it would obscure the symmetry with mojo event dispatching, making the later conversion to mojo marginally more complicated - it would make the deprecated way of doing things easier than the recommended way - it would really only save two lines of code (StartRequest/FinishRequest) and maybe one method (although it might make sense to split DeliverMessageEnd in separate success (the new OnPushEventResponse method) and failure methods anyway). https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version_unittest.cc:243: SERVICE_WORKER_ERROR_NETWORK; // dummy value On 2016/01/15 at 01:03:42, nhiroki wrote: > nit: SERVICE_WORKER_ERROR_MAX_VALUE would be more suitable for a dummy value. I use it these days. Done https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version_unittest.cc:258: EXPECT_TRUE(version_->FinishRequest(request_id)); On 2016/01/18 at 19:06:19, johnme wrote: > The previous test was actually dispatching an event, rather than merely calling StartRequest and FinishRequest. Is this still testing the same thing? Since StartRequest and FinishRequest are the only parts of dispatching an event that effect ServiceWorkerVersion, yes, this should still be the same. But I must admit that I'm not entirely sure what some of these tests are testing. Other unit tests ensure that DispatchEvent (and the equivalent mojo codepath) work correctly, so it should be safe for tests that just "dispatch an event" to not actually have any mock ipc/mojo going on. https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version_unittest.cc:813: version_->StartRequest(ServiceWorkerMetrics::EventType::PUSH, On 2016/01/18 at 19:06:19, johnme wrote: > Ditto is this testing the same thing? As I'm not entirely sure what this test is testing I'm not 100% sure, but I believe that yes, this is testing the same thing, and is doing so arguably better by only actually executing the code that matters and not also including unrelated mock IPC stuff. In particular I'm not sure if the FinishRequest calls in the second half of this test are needed. And if they are maybe the test should inspect some state before them as well (which wasn't really possible in the old test since DispatchPushEvent always got a reply more or less immediately).
https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... File content/browser/push_messaging/push_messaging_router.h (right): https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.h:40: static void DeliverMessageToWorker( On 2016/01/20 01:06:31, Marijn Kruisselbrink wrote: > On 2016/01/18 at 19:06:19, johnme wrote: > > It's kind of messy that there are now two public methods here with similar > purpose, just different levels of abstraction. > The only reason there is a second public method is because > ServiceWorkerInternalsUI needs a way to send a message to a specific worker > rather than a specific registration. Let's kill the button? I don't think anybody uses it anyway (we don't), and it's mostly duplicated with functionality in DevTools. https://codereview.chromium.org/1579413004/diff/20001/content/browser/push_me... content/browser/push_messaging/push_messaging_router.h:40: static void DeliverMessageToWorker( On 2016/01/20 01:06:31, Marijn Kruisselbrink wrote: > I could add some kind of convenience wrapper, yes... I'm not sure if it's worth > it though. StartRequest, FinishRequest, and possibly even DispatchEvent would > still have to all remain (Start and Finish for mojo based events, and > DispatchEvent because a spec compliant fetch implementation would require > possibly multiple replies to the same event; one for respondWith, and a separate > once the last waitUntil has been resolved). How common is the fetch() paradigm? As far as I'm aware, most events only require a single reply. It would make sense to optimize for that, and have fetch() be the exception case. On 2016/01/20 01:06:31, Marijn Kruisselbrink wrote: > Of course a convenience wrapper would slightly simplify the code for existing > events like push and notificationclick, but this won't effect new code at all, > since it all should be using mojo anyway (from what I heard the > tentative/aggressive plan is to completely replace IPC with mojo by the end of > the year...). Having a convenience wrapper for the deprecated code-path instead > of keeping the code closer to what it would look like in a mojo world seems a > bit odd as well. Moving from IPC to Mojo will have to be considered on a per-feature basis anyway. We plan to do this for Push, but notifications may be more difficult since we need to support a sync IPC. In the near term, a new `notificationclose` event for notification will be introduced, that might slightly increase the value of a wrapper. On 2016/01/20 01:06:31, Marijn Kruisselbrink wrote: > In this case the extra layer of indirection (at least for push) is because I > tried to keep the API exposed to service_worker_internals_ui unchanged. The code > here could be a little bit simpler by making bigger changes to > ServiceWorkerInternalsUI. As said, I think we should remove the button :-). On 2016/01/20 01:06:31, Marijn Kruisselbrink wrote: > And I'm not even sure how your suggestion to automatically transfer IPC > parameters of one type to another type in the passed callback would work? I > guess this convenience wrapper would only work if the response IPC has two > parameters, a int request_id and a blink::WebServiceWorkerEventResult? That > would already rule out using it for the notificationclick event, since that > event currently doesn't have a event result passed in its IPC (I guess template > trickery could distinguish based on the arguments of the callback, but that > complexity really doesn't seem worth it). This would be an excellent argument for making all events consistent with each other. I was wondering the other day why the `notificationclick` event doesn't share the result, it's an ExtendableEvent and it would be, at the very least, useful for UMA or smarter default behaviour - e.g. dismissing the notification automatically on failure. On 2016/01/20 01:06:31, Marijn Kruisselbrink wrote: > Partly I think why the current code seems weird is because it is doing weird > things. Why is the TRACE_EVENT1 only done if the event was dispatched more or > less succesfully? Why is it traced at all? Wouldn't it make more sense to do an > async trace from StartRequest till FinishRequest rather than for whatever reason > tracing how long handling a succesfull event takes? I think it might make a lot > of sense to remove the TRACE_EVENT parts of all the various features and instead > have TRACE_EVENT_ASYNC_BEGIN/END pairs in StartRequest/FinishRequest, but rather > than change behavior during these refactorings, I'd rather first complete the > refactoring without changing behavior and then make improvements. The trace events were added by the following patch: https://codereview.chromium.org/538913002 It looks like, right now, they're intended for manual inspection - the event's start time is tracked in the renderer process ( ServiceWorkerContextClient), the event's finish time in the browser process (now OnPushEventResponse). I'm all for having connected ASYNC traces just in the browser process, that sounds like a good improvement.
+1 to Peter's comments. https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version_unittest.cc:258: EXPECT_TRUE(version_->FinishRequest(request_id)); On 2016/01/20 01:06:31, Marijn Kruisselbrink wrote: > On 2016/01/18 at 19:06:19, johnme wrote: > > The previous test was actually dispatching an event, rather than merely > calling StartRequest and FinishRequest. Is this still testing the same thing? > > Since StartRequest and FinishRequest are the only parts of dispatching an event > that effect ServiceWorkerVersion, yes, this should still be the same. But I must > admit that I'm not entirely sure what some of these tests are testing. > > Other unit tests ensure that DispatchEvent (and the equivalent mojo codepath) > work correctly, so it should be safe for tests that just "dispatch an event" to > not actually have any mock ipc/mojo going on. Ok, since nhiroki is happy that's probably fine then :) https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version_unittest.cc:813: version_->StartRequest(ServiceWorkerMetrics::EventType::PUSH, On 2016/01/20 01:06:31, Marijn Kruisselbrink wrote: > On 2016/01/18 at 19:06:19, johnme wrote: > > Ditto is this testing the same thing? > > As I'm not entirely sure what this test is testing I'm not 100% sure, but I > believe that yes, this is testing the same thing, and is doing so arguably > better by only actually executing the code that matters and not also including > unrelated mock IPC stuff. > In particular I'm not sure if the FinishRequest calls in the second half of this > test are needed. And if they are maybe the test should inspect some state before > them as well (which wasn't really possible in the old test since > DispatchPushEvent always got a reply more or less immediately). Ok, since nhiroki is happy that's probably fine then :)
https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version_unittest.cc:258: EXPECT_TRUE(version_->FinishRequest(request_id)); On 2016/01/20 12:01:10, johnme wrote: > On 2016/01/20 01:06:31, Marijn Kruisselbrink wrote: > > On 2016/01/18 at 19:06:19, johnme wrote: > > > The previous test was actually dispatching an event, rather than merely > > calling StartRequest and FinishRequest. Is this still testing the same thing? > > > > Since StartRequest and FinishRequest are the only parts of dispatching an > event > > that effect ServiceWorkerVersion, yes, this should still be the same. But I > must > > admit that I'm not entirely sure what some of these tests are testing. > > > > Other unit tests ensure that DispatchEvent (and the equivalent mojo codepath) > > work correctly, so it should be safe for tests that just "dispatch an event" > to > > not actually have any mock ipc/mojo going on. > > Ok, since nhiroki is happy that's probably fine then :) For these StaleUpdate_* tests, StartWorker is the most important part and actually we don't have to call StartRequest/FinishRequest. StartWorker calls MarkIfStale in the middle of the sequence, and it updates |stale_time_| if registration's last update check was done over 24 hours ago. If |stale_time_| is updated, StopTimeoutTimer called from StoppedWorker starts |update_timer_|. These tests check the behavior. https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version_unittest.cc:813: version_->StartRequest(ServiceWorkerMetrics::EventType::PUSH, On 2016/01/20 12:01:10, johnme wrote: > On 2016/01/20 01:06:31, Marijn Kruisselbrink wrote: > > On 2016/01/18 at 19:06:19, johnme wrote: > > > Ditto is this testing the same thing? > > > > As I'm not entirely sure what this test is testing I'm not 100% sure, but I > > believe that yes, this is testing the same thing, and is doing so arguably > > better by only actually executing the code that matters and not also including > > unrelated mock IPC stuff. > > In particular I'm not sure if the FinishRequest calls in the second half of > this > > test are needed. And if they are maybe the test should inspect some state > before > > them as well (which wasn't really possible in the old test since > > DispatchPushEvent always got a reply more or less immediately). > > Ok, since nhiroki is happy that's probably fine then :) Hmmm... sorry, I completely overlooked this. As I explained in my other comment, StartWorker() is the most important part. This test makes sure that these start worker requests don't update |stale_time_| in MarkIfStale because it is already non-null. Probably we should call StartWorker instead of StartRequest. https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version_unittest.cc:816: base::Bind(&ServiceWorkerUtils::NoOpStatusCallback)); Can you call RunUntilIdle here? (Actually this should not be necessary because there is a ""live"" registration for the version and StartRequest can synchronously call MarkIfStale via FindRegistrationForId, but I think test code should not depend on such an internal behavior.)
Killed the "push" button in service worker internals in https://codereview.chromium.org/1617143002 I agree that most events only have one reply, so optimizing for that indeed makes sense. But I'd like for the APIs SWVersion exposes to still support more complicated events like fetch. Anyway, I tried adding a wrapper in Patch 4, but I don't really like it. The main complication is that the actual IPC message contains the request ID, but before StartRequest is called this ID isn't known yet. Hence the variadic templates in that patch to enable the wrapper method to created the actual IPC message instance. About moving from IPC to Mojo being considered on a per-feature basis, that is not my impression. From https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/Fpay4fYYjHI it seems pretty clear to me that at least some people seem to have "decided" that all of chrome should move away from IPC to Mojo, and that they hope to finish that by the end of the year. Not sure what the actual plan for that transition is and when that plan is going to be shared with the wider chromium developers community though... Anyway, in Patch 5 I went back to not having a wrapper method, and slightly reordering/cleaning up the code (for example skipping the conversion from WebServiceWorkerEventResult to ServiceWorkerStatusCode only to then convert it to PushDeliveryStatus). Not sure how it ended up as more code than before, but I don't feel like any of the boilerplate is really that bad (and it is certainly a lot better than all the boilerplate that had to be copied all over ServiceWorkerVersion in the old code). One thing that I think might be a worthwhile improvement is to change DispatchEvent to assume a single reply (and later for fetch add a separate method to dispatch events that need multiple replies). That way DispatchEvent can take care of calling FinishRequest, reducing some of the boilerplate. I don't think it is ServiceWorkerVersion's job to convert results from one data type to another data type though, so (in the current design where the datatype send over IPC doesn't match the datatype browser side push code expects) there would still be some extra code to do this conversion. Finally, I hope I changed the unit tests to now actually test what they are trying to test. https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version_unittest.cc:813: version_->StartRequest(ServiceWorkerMetrics::EventType::PUSH, On 2016/01/20 at 14:55:54, nhiroki wrote: > Hmmm... sorry, I completely overlooked this. As I explained in my other comment, StartWorker() is the most important part. This test makes sure that these start worker requests don't update |stale_time_| in MarkIfStale because it is already non-null. Probably we should call StartWorker instead of StartRequest. I changed these to call RunAfterStartWorker isntead of StartRequest. https://codereview.chromium.org/1579413004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version_unittest.cc:816: base::Bind(&ServiceWorkerUtils::NoOpStatusCallback)); On 2016/01/20 at 14:55:54, nhiroki wrote: > Can you call RunUntilIdle here? > > (Actually this should not be necessary because there is a ""live"" registration for the version and StartRequest can synchronously call MarkIfStale via FindRegistrationForId, but I think test code should not depend on such an internal behavior.) Done. Yeah, I was confused for a while how this could possibly be testing the right thing without actually waiting for possibly asynchronous stuff to complete.
> Anyway, I tried adding a wrapper in Patch 4, but I don't > really like it. The main complication is that the actual > IPC message contains the request ID, but before > StartRequest is called this ID isn't known yet. Hence the > variadic templates in that patch to enable the wrapper > method to created the actual IPC message instance. I like Patch Set 4, and prefer it to 5/6. It nicely insulates features from all the boilerplate code, making it easy to develop against Service Workers. But Patch Set 6 is also ok, if you strongly prefer it. lgtm to either. > I don't think it is ServiceWorkerVersion's job to convert > results from one data type to another data type though, so > (in the current design where the datatype send over IPC > doesn't match the datatype browser side push code expects) > there would still be some extra code to do this conversion. WebServiceWorkerEventResult is an implementation detail of how Service Workers implement waitUntil (it's not specific to push), so it seems reasonable for Service Worker code to handle that particular conversion, if it can do so easily. https://codereview.chromium.org/1579413004/diff/100001/content/browser/push_m... File content/browser/push_messaging/push_messaging_router.cc (right): https://codereview.chromium.org/1579413004/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_router.cc:95: version->RunAfterStartWorker( Nit: If you made DispatchSimpleEvent automatically handle the RunAfterStartWorker part, this could be even simpler: ServiceWorkerVersion ::DispatchSimpleEvent<ServiceWorkerMsg_PushEvent, ServiceWorkerHostMsg_PushEventFinished, std::string>( version, ServiceWorkerMetrics::EventType::PUSH, dispatch_event_callback, data); https://codereview.chromium.org/1579413004/diff/140001/content/browser/push_m... File content/browser/push_messaging/push_messaging_router.h (right): https://codereview.chromium.org/1579413004/diff/140001/content/browser/push_m... content/browser/push_messaging/push_messaging_router.h:60: // called on the IO thread, with a the worker running. s/a the/the/ https://codereview.chromium.org/1579413004/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_internals_ui.cc (right): https://codereview.chromium.org/1579413004/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_internals_ui.cc:18: #include "content/browser/push_messaging/push_messaging_router.h" Nit: Is this include still necessary?
On 2016/01/22 at 14:35:37, johnme wrote: > > Anyway, I tried adding a wrapper in Patch 4, but I don't > > really like it. The main complication is that the actual > > IPC message contains the request ID, but before > > StartRequest is called this ID isn't known yet. Hence the > > variadic templates in that patch to enable the wrapper > > method to created the actual IPC message instance. > > I like Patch Set 4, and prefer it to 5/6. It nicely insulates features from all the boilerplate code, making it easy to develop against Service Workers. But Patch Set 6 is also ok, if you strongly prefer it. lgtm to either. In the last patch I ended up somewhere in between. I added a new DispatchSimpleEvent method which combined DispatchEvent with FinishRequest in the case the event has only a ServiceWorkerEventResult as IPC response. I prefer not to also merge StartRequest into the same method due to all the template complications that brings (and the need to explicitly specify various template parameters, or have a helper method anyway, which doesn't really save any code ultimately). DispatchSimpleEvent does not currently have the tracing the old code had, but in https://codereview.chromium.org/1623583003 I'm adding async tracing for all requests. > > I don't think it is ServiceWorkerVersion's job to convert > > results from one data type to another data type though, so > > (in the current design where the datatype send over IPC > > doesn't match the datatype browser side push code expects) > > there would still be some extra code to do this conversion. > > WebServiceWorkerEventResult is an implementation detail of how Service Workers implement waitUntil (it's not specific to push), so it seems reasonable for Service Worker code to handle that particular conversion, if it can do so easily. Not entirely sure I agree with that. WebServiceWorkerEventResult in this case is just what the feature code in the renderer process happens to return... But DispatchSimpleEvent does do the conversion of WebServiceWorkerEventResult to ServiceWorkerStatusCode anyway, to keep the code simple. https://codereview.chromium.org/1579413004/diff/100001/content/browser/push_m... File content/browser/push_messaging/push_messaging_router.cc (right): https://codereview.chromium.org/1579413004/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_router.cc:95: version->RunAfterStartWorker( On 2016/01/22 at 14:35:37, johnme wrote: > Nit: If you made DispatchSimpleEvent automatically handle the RunAfterStartWorker part, this could be even simpler: > > ServiceWorkerVersion > ::DispatchSimpleEvent<ServiceWorkerMsg_PushEvent, > ServiceWorkerHostMsg_PushEventFinished, > std::string>( > version, ServiceWorkerMetrics::EventType::PUSH, > dispatch_event_callback, data); A prerequisite for that merge would be to merge RunAfterStartWorker with StartRequest. And if/when those two are merged (as in StartRequest can function without an already running worker) there really is no benefit anymore in merging StartRequest with DispatchEvent since then you could just synchronously call one after the other while SWVersion does all the correct queueing. That is something I'd love to do, but I'd prefer to wait until after I finished migrating all the events to use StartRequest so I can first simplify the request tracking in SWVersion to not have to care about multiple request types anymore. https://codereview.chromium.org/1579413004/diff/140001/content/browser/push_m... File content/browser/push_messaging/push_messaging_router.h (right): https://codereview.chromium.org/1579413004/diff/140001/content/browser/push_m... content/browser/push_messaging/push_messaging_router.h:60: // called on the IO thread, with a the worker running. On 2016/01/22 at 14:35:37, johnme wrote: > s/a the/the/ Done https://codereview.chromium.org/1579413004/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_internals_ui.cc (right): https://codereview.chromium.org/1579413004/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_internals_ui.cc:18: #include "content/browser/push_messaging/push_messaging_router.h" On 2016/01/22 at 14:35:37, johnme wrote: > Nit: Is this include still necessary? Fixed
The CQ bit was checked by mek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org, johnme@chromium.org Link to the patchset: https://codereview.chromium.org/1579413004/#ps180001 (title: "Fix DispatchSimpleEvent comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1579413004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579413004/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Move push event dispatching out of ServiceWorkerVersion. This also involves updating the various ServiceWorkerVersion unit tests to not depend on actual push events. BUG=570820 ========== to ========== Move push event dispatching out of ServiceWorkerVersion. This also involves updating the various ServiceWorkerVersion unit tests to not depend on actual push events. BUG=570820 Committed: https://crrev.com/173005c1c7134c6883c2431774c64bdde84d48c6 Cr-Commit-Position: refs/heads/master@{#371395} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/173005c1c7134c6883c2431774c64bdde84d48c6 Cr-Commit-Position: refs/heads/master@{#371395} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
