Integrate the notification database with the normal code path.

content/browser/notifications/notification_message_filter.cc

Index: content/browser/notifications/notification_message_filter.cc
diff --git a/content/browser/notifications/notification_message_filter.cc b/content/browser/notifications/notification_message_filter.cc
index f1aa17a60ab9ba7fcafbeab5f55b480cc0f109cc..9270ca878806bf2e8ecd2824720bc356e444ffd4 100644
--- a/content/browser/notifications/notification_message_filter.cc
+++ b/content/browser/notifications/notification_message_filter.cc
@@ -12,6 +12,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/desktop_notification_delegate.h"
+#include "content/public/browser/notification_database_data.h"
 #include "content/public/browser/platform_notification_service.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/content_client.h"
@@ -27,7 +28,8 @@ NotificationMessageFilter::NotificationMessageFilter(
       process_id_(process_id),
       notification_context_(notification_context),
       resource_context_(resource_context),
-      browser_context_(browser_context) {}
+      browser_context_(browser_context),
+      weak_factory_(this) {}
 
 NotificationMessageFilter::~NotificationMessageFilter() {}
 
@@ -59,24 +61,15 @@ bool NotificationMessageFilter::OnMessageReceived(const IPC::Message& message) {
 void NotificationMessageFilter::OverrideThreadForMessage(
     const IPC::Message& message, content::BrowserThread::ID* thread) {
   if (message.type() == PlatformNotificationHostMsg_Show::ID ||
-      message.type() == PlatformNotificationHostMsg_ShowPersistent::ID ||

[johnme, 2015/04/02 17:21:07]

It's a little hard to read the threading guarantees of this class. It'd probably
be helpful to do one of the following:
- Add comments in the header explaining which methods are run on what thread.
- Rename methods to indicate the thread they run on.
- Add a class-level comment explaining that methods run on both the IO and UI
threads, and readers should refer to the DCHECK_CURRENTLY_ONs.

[Peter Beverloo, 2015/04/07 17:46:11]

Could we consider this out of scope for this CL please? I plan to switch
notifications to use Mojo in M44 timeframe in either case.

-      message.type() == PlatformNotificationHostMsg_Close::ID ||
-      message.type() == PlatformNotificationHostMsg_ClosePersistent::ID)
+      message.type() == PlatformNotificationHostMsg_Close::ID)
     *thread = BrowserThread::UI;
 }
 
 void NotificationMessageFilter::OnCheckNotificationPermission(
     const GURL& origin, blink::WebNotificationPermission* permission) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  PlatformNotificationService* service =
-      GetContentClient()->browser()->GetPlatformNotificationService();
-  if (service) {
-    *permission = service->CheckPermissionOnIOThread(resource_context_,
-                                                     origin,
-                                                     process_id_);
-  } else {
-    *permission = blink::WebNotificationPermissionDenied;
-  }
+
+  *permission = GetPermissionForOriginOnIO(origin);
 }
 
 void NotificationMessageFilter::OnShowPlatformNotification(
@@ -116,25 +109,60 @@ void NotificationMessageFilter::OnShowPersistentNotification(
     const GURL& origin,
     const SkBitmap& icon,
     const PlatformNotificationData& notification_data) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (!RenderProcessHost::FromID(process_id_))
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (GetPermissionForOriginOnIO(origin) !=
+          blink::WebNotificationPermissionAllowed) {
+    BadMessageReceived();
     return;
+  }
 
-  PlatformNotificationService* service =
-      GetContentClient()->browser()->GetPlatformNotificationService();
-  DCHECK(service);
+  NotificationDatabaseData database_data;
+  database_data.origin = origin;
+  database_data.service_worker_registration_id = service_worker_registration_id;
+  database_data.notification_data = notification_data;
+
+  // TODO(peter): Significantly reduce the amount of information we need to
+  // retain outside of the database for displaying notifications.
+  notification_context_->WriteNotificationData(

[johnme, 2015/04/02 17:21:06]

Nit: It seems it might be cleaner for
PlatformNotificationService::DisplayPersistentNotification to be reponsible for
writing the data to the DB? Especially since you're passing the NotificationData
there anyway.

Similarly for ClosePersistentNotification.

[Peter Beverloo, 2015/04/07 17:46:11]

No. The Service will become dumber - it will only display or close them. This is
the first patch in a series, the following ones will change the API of PNS to
take a message_center::Notification object rather than the raw
PlatformNotificationData. However, that's a refactoring that's too big to
include in this patch.

+      origin,
+      database_data,
+      base::Bind(&NotificationMessageFilter::DidWritePersistentNotificationData,
+                 weak_factory_.GetWeakPtr(),
+                 request_id,
+                 service_worker_registration_id,
+                 origin,
+                 icon,
+                 notification_data));
+}
 
-  if (!VerifyNotificationPermissionGranted(service, origin))
-    return;
+void NotificationMessageFilter::DidWritePersistentNotificationData(
+    int request_id,
+    int64_t service_worker_registration_id,
+    const GURL& origin,
+    const SkBitmap& icon,
+    const PlatformNotificationData& notification_data,
+    bool success,
+    int64_t persistent_notification_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  service->DisplayPersistentNotification(browser_context_,
-                                         service_worker_registration_id, origin,
-                                         icon, notification_data);
+  if (success) {
+    PlatformNotificationService* service =
+      GetContentClient()->browser()->GetPlatformNotificationService();
+    DCHECK(service);
+
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&PlatformNotificationService::DisplayPersistentNotification,
+                   base::Unretained(service),  // The service is a singleton.
+                   browser_context_,
+                   persistent_notification_id,
+                   origin,
+                   icon,
+                   notification_data));
+  }
 
-  // TODO(peter): Confirm display of the persistent notification after the
-  // data has been stored using the |notification_context_|.
-  Send(new PlatformNotificationMsg_DidShowPersistent(request_id,
-                                                     true /* success */));
+  Send(new PlatformNotificationMsg_DidShowPersistent(request_id, success));
 }
 
 void NotificationMessageFilter::OnGetNotifications(
@@ -168,27 +196,65 @@ void NotificationMessageFilter::OnClosePlatformNotification(
 
 void NotificationMessageFilter::OnClosePersistentNotification(
     const GURL& origin,
-    const std::string& persistent_notification_id) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (!RenderProcessHost::FromID(process_id_))
+    int64_t persistent_notification_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (GetPermissionForOriginOnIO(origin) !=
+          blink::WebNotificationPermissionAllowed) {
+    BadMessageReceived();
     return;
+  }
 
   PlatformNotificationService* service =
       GetContentClient()->browser()->GetPlatformNotificationService();
   DCHECK(service);
 
-  // TODO(peter): Use |service_worker_registration_id| and |origin| when feeding
-  // the close event through the notification database.
+  // There's no point in waiting until the database data has been removed before
+  // closing the notification presented to the user. Post that task immediately.
+  BrowserThread::PostTask(
+      BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(&PlatformNotificationService::ClosePersistentNotification,
+                 base::Unretained(service),  // The service is a singleton.
+                 browser_context_,
+                 persistent_notification_id));
+
+  notification_context_->DeleteNotificationData(
+      persistent_notification_id,
+      origin,
+      base::Bind(&NotificationMessageFilter::
+                     DidDeletePersistentNotificationData,
+                 weak_factory_.GetWeakPtr()));
+}
 
-  service->ClosePersistentNotification(browser_context_,
-                                       persistent_notification_id);
+void NotificationMessageFilter::DidDeletePersistentNotificationData(

[johnme, 2015/04/02 17:21:07]

Nit: Please DCHECK_CURRENTLY_ON(BrowserThread::IO)

[Peter Beverloo, 2015/04/07 17:46:11]

Done.

+    bool success) {
+  // TODO(peter): Consider feeding back to the renderer that the notification

[johnme, 2015/04/02 17:21:07]

Add a TODO for UMA logging?

[Peter Beverloo, 2015/04/07 17:46:11]

We already log UMA as part of the PlatformNotificationContext. There's no need
to log more here.

+  // has been closed.
+}
+
+blink::WebNotificationPermission
+NotificationMessageFilter::GetPermissionForOriginOnIO(
+    const GURL& origin) const {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  PlatformNotificationService* service =
+      GetContentClient()->browser()->GetPlatformNotificationService();
+  if (!service)
+    return blink::WebNotificationPermissionDenied;
+
+  return service->CheckPermissionOnIOThread(resource_context_,
+                                            origin,
+                                            process_id_);
 }
 
 bool NotificationMessageFilter::VerifyNotificationPermissionGranted(

[johnme, 2015/04/02 17:21:07]

Nit: please add DCHECK_CURRENTLY_ON(BrowserThread::UI)

[Peter Beverloo, 2015/04/07 17:46:11]

Done.

     PlatformNotificationService* service,
     const GURL& origin) {
   blink::WebNotificationPermission permission =
-      service->CheckPermissionOnUIThread(browser_context_, origin, process_id_);
+      service->CheckPermissionOnUIThread(browser_context_,
+                                         origin,
+                                         process_id_);
+
   if (permission == blink::WebNotificationPermissionAllowed)
     return true;