Move push event dispatching out of ServiceWorkerVersion.

content/browser/push_messaging/push_messaging_router.h

Index: content/browser/push_messaging/push_messaging_router.h
diff --git a/content/browser/push_messaging/push_messaging_router.h b/content/browser/push_messaging/push_messaging_router.h
index dae9973de2627030fd2c66440d4a3b7434b48b51..4bae3e5efd368d79f417886637ff06963ed2f2d7 100644
--- a/content/browser/push_messaging/push_messaging_router.h
+++ b/content/browser/push_messaging/push_messaging_router.h
@@ -19,6 +19,7 @@ namespace content {
 class BrowserContext;
 class ServiceWorkerContextWrapper;
 class ServiceWorkerRegistration;
+class ServiceWorkerVersion;
 
 class PushMessagingRouter {
  public:
@@ -34,6 +35,13 @@ class PushMessagingRouter {
       const std::string& data,
       const DeliverMessageCallback& deliver_message_callback);
 
+  // Delivers a push message with |data| to a specific |service_worker|. Must be
+  // called on the IO thread.
+  static void DeliverMessageToWorker(

[johnme, 2016/01/18 19:06:19]

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.

[Marijn Kruisselbrink, 2016/01/20 01:06:31]

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.
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).

[Peter Beverloo, 2016/01/20 11:48:40]

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:
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:
As said, I think we should remove the button :-).


On 2016/01/20 01:06:31, Marijn Kruisselbrink wrote:
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:
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.

[Peter Beverloo, 2016/01/20 11:48:40]

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.

+      const scoped_refptr<ServiceWorkerVersion>& service_worker,
+      const std::string& data,
+      const base::Callback<void(ServiceWorkerStatusCode)>& callback);
+
  private:
   // Attempts to find a Service Worker registration so that a push event can be
   // dispatched. Must be called on the IO thread.