Implement notificationclose event

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 06e2cc030ec2e6b204f2aa1be758a86298a5276e..0dbde45fb05af09c3c6d2775e954b965a14943fd 100644
--- a/content/browser/notifications/notification_event_dispatcher_impl.cc
+++ b/content/browser/notifications/notification_event_dispatcher_impl.cc
@@ -12,24 +12,34 @@
 #include "content/browser/service_worker/service_worker_storage.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_database_data.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/common/platform_notification_data.h"
 
 namespace content {
 namespace {
 
-using NotificationClickDispatchCompleteCallback =
-    NotificationEventDispatcher::NotificationClickDispatchCompleteCallback;
+using NotificationDispatchCompleteCallback =
+    NotificationEventDispatcher::NotificationDispatchCompleteCallback;
+using NotificationActionCallback =
+    NotificationEventDispatcherImpl::NotificationActionCallback;
 
-// To be called when the notificationclick event has finished executing. Will
-// post a task to call |dispatch_complete_callback| on the UI thread.
-void NotificationClickEventFinished(
-    const NotificationClickDispatchCompleteCallback& dispatch_complete_callback,
-    const scoped_refptr<ServiceWorkerRegistration>& service_worker_registration,
-    ServiceWorkerStatusCode service_worker_status) {
+// To be called when a notification event has finished executing. Will post a
+// task to call |dispatch_complete_callback| on the UI thread.
+void NotificationEventFinished(
+    const NotificationDispatchCompleteCallback& dispatch_complete_callback,
+    PersistentNotificationStatus status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                          base::Bind(dispatch_complete_callback, status));
+}
+
+// To be called when a notification event has finished with a
+// ServiceWorkerStatusCode result. Will call NotificationEventFinished with a
+// PersistentNotificationStatus derived from the service worker status.
+void SWNotificationEventFinished(

[Peter Beverloo, 2016/01/26 16:13:45]

nit: s/SW/ServiceWorker/ (we don't usually use acronyms like this)

[Nina, 2016/01/27 18:48:58]

Done.

+    const NotificationDispatchCompleteCallback& dispatch_complete_callback,
+    ServiceWorkerStatusCode service_worker_status) {
 #if defined(OS_ANDROID)
   // This LOG(INFO) deliberately exists to help track down the cause of
   // https://crbug.com/534537, where notifications sometimes do not react to
@@ -68,17 +78,17 @@ void NotificationClickEventFinished(
       status = PERSISTENT_NOTIFICATION_STATUS_SERVICE_WORKER_ERROR;
       break;
   }
-
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::Bind(dispatch_complete_callback, status));
+  NotificationEventFinished(dispatch_complete_callback, status);
 }
 
-// Dispatches the notificationclick on |service_worker_registration| if the
-// registration was available. Must be called on the IO thread.
-void DispatchNotificationClickEventOnRegistration(
+// Dispatches the given notification action event on
+// |service_worker_registration| if the registration was available. Must be
+// called on the IO thread.
+void DispatchNotificationEventOnRegistration(
     const NotificationDatabaseData& notification_database_data,
-    int action_index,
-    const NotificationClickDispatchCompleteCallback& dispatch_complete_callback,
+    const scoped_refptr<PlatformNotificationContext> notification_context,
+    const NotificationActionCallback& dispatch_event_action,
+    const NotificationDispatchCompleteCallback& dispatch_error_callback,
     ServiceWorkerStatusCode service_worker_status,
     const scoped_refptr<ServiceWorkerRegistration>&
         service_worker_registration) {
@@ -88,18 +98,13 @@ void DispatchNotificationClickEventOnRegistration(
   // https://crbug.com/534537, where notifications sometimes do not react to
   // the user clicking on them. It should be removed once that's fixed.
   LOG(INFO) << "Trying to dispatch notification for SW with status: "
-            << service_worker_status << " action_index: " << action_index;
+            << service_worker_status;
 #endif
   if (service_worker_status == SERVICE_WORKER_OK) {
-    base::Callback<void(ServiceWorkerStatusCode)> 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);
+
+    dispatch_event_action.Run(service_worker_registration.get(),
+                              notification_database_data);
     return;
   }
 
@@ -135,16 +140,17 @@ void DispatchNotificationClickEventOnRegistration(
   }
 
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::Bind(dispatch_complete_callback, status));
+                          base::Bind(dispatch_error_callback, status));
 }
 
 // Finds the ServiceWorkerRegistration associated with the |origin| and
 // |service_worker_registration_id|. Must be called on the IO thread.
 void FindServiceWorkerRegistration(
     const GURL& origin,
-    int action_index,
-    const NotificationClickDispatchCompleteCallback& dispatch_complete_callback,
     scoped_refptr<ServiceWorkerContextWrapper> service_worker_context,
+    scoped_refptr<PlatformNotificationContext> notification_context,
+    const NotificationActionCallback& notification_action_callback,
+    const NotificationDispatchCompleteCallback& dispatch_error_callback,
     bool success,
     const NotificationDatabaseData& notification_database_data) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -153,22 +159,21 @@ void FindServiceWorkerRegistration(
   // This LOG(INFO) deliberately exists to help track down the cause of
   // https://crbug.com/534537, where notifications sometimes do not react to
   // the user clicking on them. It should be removed once that's fixed.
-  LOG(INFO) << "Lookup for ServiceWoker Registration: success:" << success
-            << " action_index: " << action_index;
+  LOG(INFO) << "Lookup for ServiceWoker Registration";
 #endif
   if (!success) {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
-        base::Bind(dispatch_complete_callback,
+        base::Bind(dispatch_error_callback,
                    PERSISTENT_NOTIFICATION_STATUS_DATABASE_ERROR));
     return;
   }
 
   service_worker_context->FindReadyRegistrationForId(
       notification_database_data.service_worker_registration_id, origin,
-      base::Bind(&DispatchNotificationClickEventOnRegistration,
-                 notification_database_data, action_index,
-                 dispatch_complete_callback));
+      base::Bind(&DispatchNotificationEventOnRegistration,
+                 notification_database_data, notification_context,
+                 notification_action_callback, dispatch_error_callback));
 }
 
 // Reads the data associated with the |persistent_notification_id| belonging to
@@ -176,15 +181,73 @@ void FindServiceWorkerRegistration(
 void ReadNotificationDatabaseData(
     int64_t persistent_notification_id,
     const GURL& origin,
-    int action_index,
-    const NotificationClickDispatchCompleteCallback& dispatch_complete_callback,
     scoped_refptr<ServiceWorkerContextWrapper> service_worker_context,
-    scoped_refptr<PlatformNotificationContextImpl> notification_context) {
+    scoped_refptr<PlatformNotificationContext> notification_context,
+    const NotificationActionCallback& notification_read_callback,
+    const NotificationDispatchCompleteCallback& dispatch_error_callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   notification_context->ReadNotificationData(
       persistent_notification_id, origin,
-      base::Bind(&FindServiceWorkerRegistration, origin, action_index,
-                 dispatch_complete_callback, service_worker_context));
+      base::Bind(&FindServiceWorkerRegistration, origin, service_worker_context,
+                 notification_context, notification_read_callback,
+                 dispatch_error_callback));
+}
+
+// Dispatches the notification click event on the |service_worker_registration|.
+void DoDispatchNotificationClickEvent(
+    int action_index,
+    const NotificationDispatchCompleteCallback& dispatch_complete_callback,
+    const scoped_refptr<PlatformNotificationContext> notification_context,
+    const ServiceWorkerRegistration* service_worker_registration,
+    const NotificationDatabaseData& notification_database_data) {
+  service_worker_registration->active_version()->DispatchNotificationClickEvent(
+      base::Bind(&SWNotificationEventFinished, dispatch_complete_callback),
+      notification_database_data.notification_id,
+      notification_database_data.notification_data, action_index);
+}
+
+// Called when the notification data has been deleted to finish the notification
+// close event.
+void OnPersistentNotificationDataDeleted(
+    const NotificationDispatchCompleteCallback& dispatch_complete_callback,
+    bool success) {
+  if (success) {

[Peter Beverloo, 2016/01/26 16:13:45]

You could reduce duplication by branching for the arguments only -

NotificationEventFinished(
    dispatch_complete_callback,
    success ? PERSISTENT_NOTIFICATION_STATUS_SUCCESS
            : PERSISTENT_NOTIFICATION_STATUS_DATABASE_ERROR);

[Nina, 2016/01/27 18:48:58]

Done.

+    NotificationEventFinished(dispatch_complete_callback,
+                              PERSISTENT_NOTIFICATION_STATUS_SUCCESS);
+  } else {
+    NotificationEventFinished(dispatch_complete_callback,
+                              PERSISTENT_NOTIFICATION_STATUS_DATABASE_ERROR);
+  }
+}
+
+// Called when the persistent notification close event has been handled
+// to remove the notification from the database.
+void OnDidDispatchNotificationCloseEvent(
+    const int64_t notification_id,
+    const GURL& origin,
+    scoped_refptr<PlatformNotificationContext> notification_context,
+    const NotificationDispatchCompleteCallback& dispatch_complete_callback,
+    ServiceWorkerStatusCode status_code) {

[Peter Beverloo, 2016/01/26 16:13:45]

We'll want to return the result status of dispatching the close event in this
case as well, e.g. consider |status_code| like SWNotificationEventFinished()
does. That would allow for more comprehensive UMA in the //chrome layer. WDYT?

[Nina, 2016/01/27 18:48:58]

Sounds good. Since we want to call the callback only once per event dispatch, I
changed the code to run the callback with the error code if we get one here and
ignore any DB error later.

+  notification_context->DeleteNotificationData(
+      notification_id, origin, base::Bind(&OnPersistentNotificationDataDeleted,
+                                          dispatch_complete_callback));
+}
+
+// Actually dispatches the notification close event on the service worker
+// registration.
+void DoDispatchNotificationCloseEvent(
+    const NotificationDispatchCompleteCallback& dispatch_complete_callback,
+    scoped_refptr<PlatformNotificationContext> notification_context,
+    const ServiceWorkerRegistration* service_worker_registration,
+    const NotificationDatabaseData& notification_database_data) {
+  const base::Callback<void(ServiceWorkerStatusCode)> dispatch_event_callback =
+      base::Bind(&OnDidDispatchNotificationCloseEvent,
+                 notification_database_data.notification_id,
+                 notification_database_data.origin, notification_context,
+                 dispatch_complete_callback);
+  service_worker_registration->active_version()->DispatchNotificationCloseEvent(
+      dispatch_event_callback, notification_database_data.notification_id,
+      notification_database_data.notification_data);
 }
 
 }  // namespace
@@ -209,8 +272,31 @@ void NotificationEventDispatcherImpl::DispatchNotificationClickEvent(
     int64_t persistent_notification_id,
     const GURL& origin,
     int action_index,
-    const NotificationClickDispatchCompleteCallback&
-        dispatch_complete_callback) {
+    const NotificationDispatchCompleteCallback& dispatch_complete_callback) {
+  DispatchNotificationEvent(
+      browser_context, persistent_notification_id, origin,
+      base::Bind(&DoDispatchNotificationClickEvent, action_index,
+                 dispatch_complete_callback),
+      dispatch_complete_callback);
+}
+
+void NotificationEventDispatcherImpl::DispatchNotificationCloseEvent(
+    BrowserContext* browser_context,
+    int64_t persistent_notification_id,
+    const GURL& origin,
+    const NotificationDispatchCompleteCallback& dispatch_complete_callback) {
+  DispatchNotificationEvent(
+      browser_context, persistent_notification_id, origin,
+      base::Bind(&DoDispatchNotificationCloseEvent, dispatch_complete_callback),
+      dispatch_complete_callback);
+}
+
+void NotificationEventDispatcherImpl::DispatchNotificationEvent(
+    BrowserContext* browser_context,
+    int64_t persistent_notification_id,
+    const GURL& origin,
+    const CurriedNotificationActionCallback& notification_action_callback,
+    const NotificationDispatchCompleteCallback& notification_error_callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_GT(persistent_notification_id, 0);
   DCHECK(origin.is_valid());
@@ -221,15 +307,15 @@ void NotificationEventDispatcherImpl::DispatchNotificationClickEvent(
   scoped_refptr<ServiceWorkerContextWrapper> service_worker_context =
       static_cast<ServiceWorkerContextWrapper*>(
           partition->GetServiceWorkerContext());
-  scoped_refptr<PlatformNotificationContextImpl> notification_context =
-      static_cast<PlatformNotificationContextImpl*>(
-          partition->GetPlatformNotificationContext());
+  scoped_refptr<PlatformNotificationContext> notification_context =
+      partition->GetPlatformNotificationContext();
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&ReadNotificationDatabaseData, persistent_notification_id,
-                 origin, action_index, dispatch_complete_callback,
-                 service_worker_context, notification_context));
+                 origin, service_worker_context, notification_context,
+                 base::Bind(notification_action_callback, notification_context),
+                 notification_error_callback));
 }
 
 }  // namespace content