Move notification click event dispatching out of ServiceWorkerVersion.

BUG=570820

Review URL: https://codereview.chromium.org/1575283003

Cr-Commit-Position: refs/heads/master@{#371559}

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"
@@ -73,6 +74,24 @@ void NotificationClickEventFinished(
                           base::Bind(dispatch_complete_callback, status));
 }
 
+// Dispatches the notificationclick event on |service_worker|. Most be called on
+// the IO thread, and with the worker running.
+void DispatchNotificationClickEventOnWorker(
+    const scoped_refptr<ServiceWorkerVersion>& service_worker,
+    const NotificationDatabaseData& notification_database_data,
+    int action_index,
+    const ServiceWorkerVersion::StatusCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  int request_id = service_worker->StartRequest(
+      ServiceWorkerMetrics::EventType::NOTIFICATION_CLICK, callback);
+  service_worker->DispatchSimpleEvent<
+      ServiceWorkerHostMsg_NotificationClickEventFinished>(
+      request_id,
+      ServiceWorkerMsg_NotificationClickEvent(
+          request_id, notification_database_data.notification_id,
+          notification_database_data.notification_data, action_index));
+}
+
 // Dispatches the notificationclick on |service_worker_registration| if the
 // registration was available. Must be called on the IO thread.
 void DispatchNotificationClickEventOnRegistration(
@@ -91,15 +110,17 @@ void DispatchNotificationClickEventOnRegistration(
             << service_worker_status << " action_index: " << action_index;
 #endif
   if (service_worker_status == SERVICE_WORKER_OK) {
-    base::Callback<void(ServiceWorkerStatusCode)> dispatch_event_callback =
+    ServiceWorkerVersion::StatusCallback dispatch_event_callback =
         base::Bind(&NotificationClickEventFinished, dispatch_complete_callback,
                    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(
+        base::Bind(
+            &DispatchNotificationClickEventOnWorker,
+            make_scoped_refptr(service_worker_registration->active_version()),
+            notification_database_data, action_index, dispatch_event_callback),
+        dispatch_event_callback);
     return;
   }
 

content/browser/service_worker/service_worker_version.cc

@@ -654,37 +654,6 @@ void ServiceWorkerVersion::DispatchFetchEvent(
   }
 }
 
-void ServiceWorkerVersion::DispatchNotificationClickEvent(
-    const StatusCallback& callback,
-    int64_t persistent_notification_id,
-    const PlatformNotificationData& notification_data,
-    int action_index) {
-  OnBeginEvent();
-  DCHECK_EQ(ACTIVATED, status()) << status();
-  if (running_status() != RUNNING) {
-    // Schedule calling this method after starting the worker.
-    StartWorker(base::Bind(
-        &RunTaskAfterStartWorker, weak_factory_.GetWeakPtr(), callback,
-        base::Bind(&self::DispatchNotificationClickEvent,
-                   weak_factory_.GetWeakPtr(), callback,
-                   persistent_notification_id, notification_data,
-                   action_index)));
-    return;
-  }
-
-  int request_id = AddRequest(
-      callback, &notification_click_requests_, REQUEST_NOTIFICATION_CLICK,
-      ServiceWorkerMetrics::EventType::NOTIFICATION_CLICK);
-  ServiceWorkerStatusCode status =
-      embedded_worker_->SendMessage(ServiceWorkerMsg_NotificationClickEvent(
-          request_id, persistent_notification_id, notification_data,
-          action_index));
-  if (status != SERVICE_WORKER_OK) {
-    notification_click_requests_.Remove(request_id);
-    RunSoon(base::Bind(callback, status));
-  }
-}
-
 void ServiceWorkerVersion::DispatchCrossOriginMessageEvent(
     const NavigatorConnectClient& client,
     const base::string16& message,
@@ -994,8 +963,6 @@ bool ServiceWorkerVersion::OnMessageReceived(const IPC::Message& message) {
                         OnInstallEventFinished)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_FetchEventFinished,
                         OnFetchEventFinished)
-    IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_NotificationClickEventFinished,
-                        OnNotificationClickEventFinished)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_OpenWindow,
                         OnOpenWindow)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_SetCachedMetadata,
@@ -1165,26 +1132,6 @@ void ServiceWorkerVersion::OnFetchEventFinished(
   RemoveCallbackAndStopIfRedundant(&fetch_requests_, request_id);
 }
 
-void ServiceWorkerVersion::OnNotificationClickEventFinished(
-    int request_id) {
-  TRACE_EVENT1("ServiceWorker",
-               "ServiceWorkerVersion::OnNotificationClickEventFinished",
-               "Request id", request_id);
-  PendingRequest<StatusCallback>* request =
-      notification_click_requests_.Lookup(request_id);
-  if (!request) {
-    NOTREACHED() << "Got unexpected message: " << request_id;
-    return;
-  }
-
-  ServiceWorkerMetrics::RecordEventDuration(
-      request->event_type, base::TimeTicks::Now() - request->start_time);
-
-  scoped_refptr<ServiceWorkerVersion> protect(this);
-  request->callback.Run(SERVICE_WORKER_OK);
-  RemoveCallbackAndStopIfRedundant(&notification_click_requests_, request_id);
-}
-
 void ServiceWorkerVersion::OnSimpleEventResponse(
     int request_id,
     blink::WebServiceWorkerEventResult result) {
@@ -1736,7 +1683,6 @@ void ServiceWorkerVersion::StopWorkerIfIdle() {
 bool ServiceWorkerVersion::HasInflightRequests() const {
   return !activate_requests_.IsEmpty() || !install_requests_.IsEmpty() ||
          !fetch_requests_.IsEmpty() ||
-         !notification_click_requests_.IsEmpty() ||
          !custom_requests_.IsEmpty() || !streaming_url_request_jobs_.empty();
 }
 
@@ -1843,9 +1789,6 @@ bool ServiceWorkerVersion::MaybeTimeOutRequest(const RequestInfo& info) {
           &fetch_requests_, info.id, SERVICE_WORKER_ERROR_TIMEOUT,
           /* The other args are ignored for non-OK status. */
           SERVICE_WORKER_FETCH_EVENT_RESULT_FALLBACK, ServiceWorkerResponse());
-    case REQUEST_NOTIFICATION_CLICK:
-      return RunIDMapCallback(&notification_click_requests_, info.id,
-                              SERVICE_WORKER_ERROR_TIMEOUT);
     case REQUEST_CUSTOM:
       return RunIDMapCallback(&custom_requests_, info.id,
                               SERVICE_WORKER_ERROR_TIMEOUT);
@@ -1964,7 +1907,6 @@ void ServiceWorkerVersion::OnStoppedInternal(
   RunIDMapCallbacks(&fetch_requests_, SERVICE_WORKER_ERROR_FAILED,
                     SERVICE_WORKER_FETCH_EVENT_RESULT_FALLBACK,
                     ServiceWorkerResponse());
-  RunIDMapCallbacks(&notification_click_requests_, SERVICE_WORKER_ERROR_FAILED);
   RunIDMapCallbacks(&custom_requests_, SERVICE_WORKER_ERROR_FAILED);
 
   // Close all mojo services. This will also fire and clear all callbacks

content/browser/service_worker/service_worker_version.h

@@ -53,7 +53,6 @@ class ServiceWorkerProviderHost;
 class ServiceWorkerRegistration;
 class ServiceWorkerURLRequestJob;
 struct NavigatorConnectClient;
-struct PlatformNotificationData;
 struct ServiceWorkerClientInfo;
 struct ServiceWorkerVersionInfo;
 struct TransferredMessagePort;
@@ -271,17 +270,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
                           const base::Closure& prepare_callback,
                           const FetchCallback& fetch_callback);
 
-  // Sends notificationclick event to the associated embedded worker and
-  // asynchronously calls |callback| when it errors out or it gets a response
-  // from the worker to notify completion.
-  //
-  // This must be called when the status() is ACTIVATED.
-  void DispatchNotificationClickEvent(
-      const StatusCallback& callback,
-      int64_t persistent_notification_id,
-      const PlatformNotificationData& notification_data,
-      int action_index);
-
   // Sends a cross origin message event to the associated embedded worker and
   // asynchronously calls |callback| when the message was sent (or failed to
   // sent).
@@ -409,7 +397,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
     REQUEST_ACTIVATE,
     REQUEST_INSTALL,
     REQUEST_FETCH,
-    REQUEST_NOTIFICATION_CLICK,
     REQUEST_CUSTOM,
     NUM_REQUEST_TYPES
   };
@@ -570,7 +557,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
   void OnFetchEventFinished(int request_id,
                             ServiceWorkerFetchEventResult result,
                             const ServiceWorkerResponse& response);
-  void OnNotificationClickEventFinished(int request_id);
   void OnSimpleEventResponse(int request_id,
                              blink::WebServiceWorkerEventResult result);
   void OnOpenWindow(int request_id, GURL url);
@@ -699,8 +685,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
   IDMap<PendingRequest<StatusCallback>, IDMapOwnPointer> activate_requests_;
   IDMap<PendingRequest<StatusCallback>, IDMapOwnPointer> install_requests_;
   IDMap<PendingRequest<FetchCallback>, IDMapOwnPointer> fetch_requests_;
-  IDMap<PendingRequest<StatusCallback>, IDMapOwnPointer>
-      notification_click_requests_;
   IDMap<PendingRequest<StatusCallback>, IDMapOwnPointer> custom_requests_;
 
   // Stores all open connections to mojo services. Maps the service name to
@@ -837,6 +821,9 @@ bool ServiceWorkerVersion::EventResponseHandler<ResponseMessage, CallbackType>::
                                  &CallbackType::Run))
     message.set_dispatch_error();
 
+  // At this point |this| can have been deleted, so don't do anything other
+  // than returning.
+
   return true;
 }
 

content/common/service_worker/service_worker_messages.h

@@ -235,8 +235,9 @@ IPC_MESSAGE_ROUTED3(ServiceWorkerHostMsg_FetchEventFinished,
                     int /* request_id */,
                     content::ServiceWorkerFetchEventResult,
                     content::ServiceWorkerResponse)
-IPC_MESSAGE_ROUTED1(ServiceWorkerHostMsg_NotificationClickEventFinished,
-                    int /* request_id */)
+IPC_MESSAGE_ROUTED2(ServiceWorkerHostMsg_NotificationClickEventFinished,
+                    int /* request_id */,
+                    blink::WebServiceWorkerEventResult)
 IPC_MESSAGE_ROUTED2(ServiceWorkerHostMsg_PushEventFinished,
                     int /* request_id */,
                     blink::WebServiceWorkerEventResult)

content/renderer/service_worker/service_worker_context_client.cc

@@ -521,7 +521,7 @@ void ServiceWorkerContextClient::didHandleNotificationClickEvent(
     int request_id,
     blink::WebServiceWorkerEventResult result) {
   Send(new ServiceWorkerHostMsg_NotificationClickEventFinished(
-      GetRoutingID(), request_id));
+      GetRoutingID(), request_id, result));
 }
 
 void ServiceWorkerContextClient::didHandlePushEvent(