Move notification click event dispatching out of ServiceWorkerVersion.

content/browser/notifications/notification_event_dispatcher_impl.cc

Index: content/browser/notifications/notification_event_dispatcher_impl.cc
diff --git a/content/browser/notifications/notification_event_dispatcher_impl.cc b/content/browser/notifications/notification_event_dispatcher_impl.cc
index d48491ce2b8601fec05914a56db6f459fb435cf5..54a5906f30d45a1722757e318e94aba0d4df2478 100644
--- a/content/browser/notifications/notification_event_dispatcher_impl.cc
+++ b/content/browser/notifications/notification_event_dispatcher_impl.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/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_database_data.h"
@@ -72,6 +73,33 @@ void NotificationClickEventFinished(
                           base::Bind(dispatch_complete_callback, status));
 }
 
+void OnNotificationClickEventReply(

[Michael van Ouwerkerk, 2016/01/15 16:44:03]

nit: add some documentation like the other functions

[Marijn Kruisselbrink, 2016/01/15 23:48:49]

Done

+    const scoped_refptr<ServiceWorkerVersion>& service_worker,
+    const base::Callback<void(ServiceWorkerStatusCode)>& callback,

[johnme, 2016/01/15 16:52:58]

Nit: may as well use `const ServiceWorkerVersion::StatusCallback&` since this
has to match the first parameter of RunAfterStartWorker.

[Marijn Kruisselbrink, 2016/01/15 23:48:49]

Agreed; not sure why the code in DispatchNotificationClickEventOnRegistration
used the explicit base::Callback type rather than just using
SWVersion::StatusCallback. Changed it there too for consistency.

+    int request_id) {

[Michael van Ouwerkerk, 2016/01/15 16:44:03]

nit: DCHECK_CURRENTLY_ON like all other functions in this file

[Marijn Kruisselbrink, 2016/01/15 23:48:49]

Done

+  TRACE_EVENT1("ServiceWorker",
+               "NotificationEventDispatcherImpl::OnNotificationClickEventReply",
+               "Request id", request_id);
+  service_worker->FinishRequest(request_id);

[johnme, 2016/01/15 16:52:58]

Please NOTREACHED() if this returns false.

[Marijn Kruisselbrink, 2016/01/15 23:48:49]

Done

+  callback.Run(SERVICE_WORKER_OK);

[johnme, 2016/01/15 16:52:58]

The previous code did the following:

scoped_refptr<ServiceWorkerVersion> protect(this);	
request->callback.Run(SERVICE_WORKER_OK);	
RemoveCallbackAndStopIfRedundant(&notification_click_requests_, request_id);

In order to explicitly keep the Service Worker alive whilst the callback was
running. But here, you don't have a protect refptr, and instead you call
FinishRequest - and hence RemoveCallbackAndStopIfRedundant - before calling the
callback.

Is this still safe?

[Marijn Kruisselbrink, 2016/01/15 23:48:49]

If I understand it correctly, the old code made sure to keep the SWVersion alive
not so much for the benefit of the callback, but to make sure the subsequent
RemoveCallbackAndStopIfRedundant call could still proceed and |this| would still
be valid at that point (and maybe as well to make sure the actual callback
wouldn't get deleted while it was running). 
So I believe the current code is just as safe: to be able to call FinishRequest
code must have a valid ServiceWorkerVersion pointer, and nothing FinishRequest
does could cause the SWVersion to actually get deleted. Also the actual method
here where FinishRequest is called from has a scoped_refptr to the
ServiceWorkerVersion, so even if that's the only reference left it would still
survive past calling the callback.

[johnme, 2016/01/18 16:31:05]

You're right - the (somewhat awkward) pattern is for the callback to be bound
with a scoped_refptr to the SW (just to keep it alive - the callback doesn't use
it), so once the callback finishes the refcount is decremented and the SW might
be destroyed.
Indeed, this seems just as safe. I wonder if it might perform slightly worse, in
the (currently theoretical) case where the callback synchronously dispatches a
new event to the SW? With the new code, it's possible that the SW would be
stopped due to idleness before the callback is run, and hence the SW would have
to be restarted. But feel free to keep the new order if it has benefits.

+}
+
+void DispatchNotificationClickEventOnWorker(

[Michael van Ouwerkerk, 2016/01/15 16:44:03]

nit: add some documentation like the other functions

[Marijn Kruisselbrink, 2016/01/15 23:48:49]

Done

+    const scoped_refptr<ServiceWorkerVersion>& service_worker,
+    const NotificationDatabaseData& notification_database_data,
+    int action_index,
+    const base::Callback<void(ServiceWorkerStatusCode)>& callback) {

[Michael van Ouwerkerk, 2016/01/15 16:44:03]

nit: DCHECK_CURRENTLY_ON like all other functions in this file

[johnme, 2016/01/15 16:52:58]

Ditto `const ServiceWorkerVersion::StatusCallback&`

[Marijn Kruisselbrink, 2016/01/15 23:48:49]

Done

+  int request_id = service_worker->StartRequest(
+      ServiceWorkerMetrics::EventType::NOTIFICATION_CLICK, callback);
+  service_worker
+      ->DispatchEvent<ServiceWorkerHostMsg_NotificationClickEventFinished>(
+          request_id,
+          ServiceWorkerMsg_NotificationClickEvent(
+              request_id, notification_database_data.notification_id,
+              notification_database_data.notification_data, action_index),
+          base::Bind(&OnNotificationClickEventReply, service_worker, callback));
+}
+
 // Dispatches the notificationclick on |service_worker_registration| if the
 // registration was available. Must be called on the IO thread.
 void DispatchNotificationClickEventOnRegistration(
@@ -95,10 +123,12 @@ void DispatchNotificationClickEventOnRegistration(
                    service_worker_registration);
 
     DCHECK(service_worker_registration->active_version());
-    service_worker_registration->active_version()
-        ->DispatchNotificationClickEvent(
-            dispatch_event_callback, notification_database_data.notification_id,
-            notification_database_data.notification_data, action_index);
+    service_worker_registration->active_version()->RunAfterStartWorker(

[johnme, 2016/01/15 16:52:58]

Is it ok to call RunAfterStartWorker (and hence StartWorker) if the SW is
already running? ServiceWorkerVersion::DispatchNotificationClickEvent would only
call StartWorker if it wasn't already running.

For example some new side-effects that I noticed are that StartWorker will
overwrite prestart_status_, and DidEnsureLiveRegistrationForStartWorker will
call MarkIfStale.

[Marijn Kruisselbrink, 2016/01/15 23:48:49]

Good question; I don't think the side effects will be problematic, but no need
to actually change behavior for this, so in
https://codereview.chromium.org/1584653009 I changed the RunAfterStartWorker
implementation to match its description better and only call StartWorker if the
worker isn't already running.

+        dispatch_event_callback,

[Michael van Ouwerkerk, 2016/01/15 16:44:03]

nit: I would expect the error callback argument to go after the main task
argument, like in most code I've seen elsewhere. I appreciate that
RunAfterStartWorker is an existing API so it might not be appropriate to change
in this CL.

[Marijn Kruisselbrink, 2016/01/15 23:48:49]

Changed this order in https://codereview.chromium.org/1584653009 as well (and
will of course make sure to update whichever CL lands second to include changes
from the other).

+        base::Bind(
+            &DispatchNotificationClickEventOnWorker,
+            make_scoped_refptr(service_worker_registration->active_version()),
+            notification_database_data, action_index, dispatch_event_callback));
     return;
   }