Move notification click event dispatching out of ServiceWorkerVersion.

content/browser/service_worker/service_worker_version.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/service_worker_version.h"
 
 #include <stddef.h>
 
 #include <map>
 #include <string>
@@ skipping 607 matching lines @@
   ServiceWorkerStatusCode status = embedded_worker_->SendMessage(
       ServiceWorkerMsg_FetchEvent(request_id, request));
   if (status != SERVICE_WORKER_OK) {
     fetch_requests_.Remove(request_id);
     RunSoon(base::Bind(&RunErrorFetchCallback,
                        fetch_callback,
                        SERVICE_WORKER_ERROR_FAILED));
   }
 }
 
-void ServiceWorkerVersion::DispatchNotificationClickEvent(
-    const StatusCallback& callback,
-    int64_t persistent_notification_id,
-    const PlatformNotificationData& notification_data,
-    int action_index) {
-  OnBeginEvent();
-  DCHECK_EQ(ACTIVATED, status()) << status();

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

Is there any equivalent to this DCHECK in the new version?

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

Good question. Not currently. I considered adding something like DCHECK(status()
== ACTIVATED || event_type == INSTALL || event_type == ACTIVATE || event_type ==
MESSAGE) to StartRequest, but that didn't feel right to me. Also with the
changes to ServiceWorkerContextWrapper were it now generally will only return a
registration with a fully activated worker (unless you call GetAllRegistrations)
it seemed the risk of triggering this DCHECK is much lower than what it was when
this code was originally written.

But I could easily be convinced that a check like this is still meaningful. I'm
not sure if it should be in feature specific code (where it would be checking
that FindReadyRegistration* in fact did return a fully activated worker) or if
the check should go in StartRequest (where I'd rather not hardcode the list of
events that can be send to a worker that isn't active).

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

Both push and notifications have experienced a lot of subtle bugs about Service
Workers not being in the right state. I'd rather keep this check in some form.
Having a whitelist of events which don't need an activated Service Worker
actually sounds quite reasonable, since this should be a small finite list
(possibly just INSTALL and ACTIVATE? Why would we dispatch a MESSAGE to an
inactive SW?).

[Marijn Kruisselbrink, 2016/01/19 23:50:17]

I think most of the subtle bugs have been adressed by the changes in
ServiceWorkerContextWrapper, that pretty much now makes it impossible to get a
worker which isn't in the right state. But given that the "handle functional
event" algorithm does require an activating/activated service worker, I agree
that the list of events without this requirements is likely going to be small. I
created https://codereview.chromium.org/1604033003 to add this DCHECK.

(We would dispatch a MESSAGE to an inactive SW because the spec says we should
allow this (for example to allow communication between installing page and
installing service worker during the oninstall event)).

-  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::DispatchPushEvent(const StatusCallback& callback,
                                              const std::string& data) {
   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::DispatchPushEvent,
                                       weak_factory_.GetWeakPtr(),
@@ skipping 306 matching lines @@
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(ServiceWorkerVersion, message)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_GetClients,
                         OnGetClients)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_ActivateEventFinished,
                         OnActivateEventFinished)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_InstallEventFinished,
                         OnInstallEventFinished)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_FetchEventFinished,
                         OnFetchEventFinished)
-    IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_NotificationClickEventFinished,
-                        OnNotificationClickEventFinished)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_PushEventFinished,
                         OnPushEventFinished)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_OpenWindow,
                         OnOpenWindow)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_SetCachedMetadata,
                         OnSetCachedMetadata)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_ClearCachedMetadata,
                         OnClearCachedMetadata)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_PostMessageToClient,
                         OnPostMessageToClient)
@@ skipping 151 matching lines @@
                                      handled);
 
   ServiceWorkerMetrics::RecordFetchEventTime(
       result, base::TimeTicks::Now() - request->start_time);
 
   scoped_refptr<ServiceWorkerVersion> protect(this);
   request->callback.Run(SERVICE_WORKER_OK, result, response);
   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::OnPushEventFinished(
     int request_id,
     blink::WebServiceWorkerEventResult result) {
   TRACE_EVENT1("ServiceWorker",
                "ServiceWorkerVersion::OnPushEventFinished",
                "Request id", request_id);
   PendingRequest<StatusCallback>* request = push_requests_.Lookup(request_id);
   if (!request) {
     NOTREACHED() << "Got unexpected message: " << request_id;
     return;
@@ skipping 537 matching lines @@
   if (running_status() == STOPPED || running_status() == STOPPING ||
       !stop_callbacks_.empty()) {
     return;
   }
 
   embedded_worker_->StopIfIdle();
 }
 
 bool ServiceWorkerVersion::HasInflightRequests() const {
   return !activate_requests_.IsEmpty() || !install_requests_.IsEmpty() ||
-         !fetch_requests_.IsEmpty() ||
-         !notification_click_requests_.IsEmpty() || !push_requests_.IsEmpty() ||
+         !fetch_requests_.IsEmpty() || !push_requests_.IsEmpty() ||
          !custom_requests_.IsEmpty() || !streaming_url_request_jobs_.empty();
 }
 
 void ServiceWorkerVersion::RecordStartWorkerResult(
     ServiceWorkerStatusCode status) {
   base::TimeTicks start_time = start_time_;
   ClearTick(&start_time_);
 
   ServiceWorkerMetrics::RecordStartWorkerStatus(status,
                                                 IsInstalled(prestart_status_));
@@ skipping 74 matching lines @@
       return RunIDMapCallback(&activate_requests_, info.id,
                               SERVICE_WORKER_ERROR_TIMEOUT);
     case REQUEST_INSTALL:
       return RunIDMapCallback(&install_requests_, info.id,
                               SERVICE_WORKER_ERROR_TIMEOUT);
     case REQUEST_FETCH:
       return RunIDMapCallback(
           &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_PUSH:
       return RunIDMapCallback(&push_requests_, info.id,
                               SERVICE_WORKER_ERROR_TIMEOUT);
     case REQUEST_CUSTOM:
       return RunIDMapCallback(&custom_requests_, info.id,
                               SERVICE_WORKER_ERROR_TIMEOUT);
     case NUM_REQUEST_TYPES:
       break;
   }
   NOTREACHED() << "Got unexpected request type: " << info.type;
@@ skipping 101 matching lines @@
   // Let all message callbacks fail (this will also fire and clear all
   // callbacks for events).
   // TODO(kinuko): Consider if we want to add queue+resend mechanism here.
   RunIDMapCallbacks(&activate_requests_,
                     SERVICE_WORKER_ERROR_ACTIVATE_WORKER_FAILED);
   RunIDMapCallbacks(&install_requests_,
                     SERVICE_WORKER_ERROR_INSTALL_WORKER_FAILED);
   RunIDMapCallbacks(&fetch_requests_, SERVICE_WORKER_ERROR_FAILED,
                     SERVICE_WORKER_FETCH_EVENT_RESULT_FALLBACK,
                     ServiceWorkerResponse());
-  RunIDMapCallbacks(&notification_click_requests_, SERVICE_WORKER_ERROR_FAILED);
   RunIDMapCallbacks(&push_requests_, SERVICE_WORKER_ERROR_FAILED);
   RunIDMapCallbacks(&custom_requests_, SERVICE_WORKER_ERROR_FAILED);
 
   // Close all mojo services. This will also fire and clear all callbacks
   // for messages that are still outstanding for those services.
   mojo_services_.clear();
 
   // TODO(falken): Call SWURLRequestJob::ClearStream here?
   streaming_url_request_jobs_.clear();
 
@@ skipping 12 matching lines @@
 void ServiceWorkerVersion::OnBeginEvent() {
   if (should_exclude_from_uma_ || running_status() != RUNNING ||
       idle_time_.is_null()) {
     return;
   }
   ServiceWorkerMetrics::RecordTimeBetweenEvents(base::TimeTicks::Now() -
                                                 idle_time_);
 }
 
 }  // namespace content