Move push event dispatching out of ServiceWorkerVersion.

content/browser/push_messaging/push_messaging_router.cc

Index: content/browser/push_messaging/push_messaging_router.cc
diff --git a/content/browser/push_messaging/push_messaging_router.cc b/content/browser/push_messaging/push_messaging_router.cc
index bc95216ad07698514323caf241f6a1542273660d..822bc299687480fb5629ff99df90d5117572d1fa 100644
--- a/content/browser/push_messaging/push_messaging_router.cc
+++ b/content/browser/push_messaging/push_messaging_router.cc
@@ -10,6 +10,7 @@
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_registration.h"
 #include "content/browser/service_worker/service_worker_storage.h"
+#include "content/common/service_worker/service_worker_messages.h"
 #include "content/common/service_worker/service_worker_status_code.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
@@ -27,6 +28,21 @@ void RunDeliverCallback(
       base::Bind(deliver_message_callback, delivery_status));
 }
 
+void OnPushEventResponse(scoped_refptr<ServiceWorkerVersion> service_worker,

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

[see comment in .h first]

Please add DCHECK_CURRENTLY_ON(BrowserThread::IO).

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

Done

+                         base::Callback<void(ServiceWorkerStatusCode)> callback,
+                         int request_id,
+                         blink::WebServiceWorkerEventResult result) {
+  TRACE_EVENT1("ServiceWorker", "PushMessagingRouter::OnPushEventResponse",
+               "Request id", request_id);
+
+  service_worker->FinishRequest(request_id);

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

[see comment in .h first]

Please apply equivalent tweaks as in https://codereview.chromium.org/1575283003,
e.g. check the return value of FinishEvent.

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

Done

+
+  ServiceWorkerStatusCode status = SERVICE_WORKER_OK;
+  if (result == blink::WebServiceWorkerEventResultRejected)
+    status = SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED;
+  callback.Run(status);
+}
+
 }  // namespace
 
 // static
@@ -50,6 +66,26 @@ void PushMessagingRouter::DeliverMessage(
 }
 
 // static
+void PushMessagingRouter::DeliverMessageToWorker(

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

[see comment in .h first]

Nit: please move this between
PushMessagingRouter::FindServiceWorkerRegistrationCallback and
PushMessagingRouter::DeliverMessageEnd to keep them in order of execution.

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

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

+    const scoped_refptr<ServiceWorkerVersion>& service_worker,
+    const std::string& data,
+    const base::Callback<void(ServiceWorkerStatusCode)>& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (service_worker->running_status() != ServiceWorkerVersion::RUNNING) {

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

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

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

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

+    service_worker->RunAfterStartWorker(
+        callback, base::Bind(&PushMessagingRouter::DeliverMessageToWorker,
+                             service_worker, data, callback));
+    return;
+  }
+
+  int request_id = service_worker->StartRequest(
+      ServiceWorkerMetrics::EventType::PUSH, callback);
+  service_worker->DispatchEvent<ServiceWorkerHostMsg_PushEventFinished>(
+      request_id, ServiceWorkerMsg_PushEvent(request_id, data),
+      base::Bind(&OnPushEventResponse, service_worker, callback));
+}
+
+// static
 void PushMessagingRouter::FindServiceWorkerRegistration(
     const GURL& origin,
     int64_t service_worker_registration_id,
@@ -90,8 +126,7 @@ void PushMessagingRouter::FindServiceWorkerRegistrationCallback(
   base::Callback<void(ServiceWorkerStatusCode)> dispatch_event_callback =
       base::Bind(&PushMessagingRouter::DeliverMessageEnd,
                  deliver_message_callback, service_worker_registration);
-
-  version->DispatchPushEvent(dispatch_event_callback, data);
+  DeliverMessageToWorker(version, data, dispatch_event_callback);
 }
 
 // static