[WebAPKs] Plumb service worker scope to notifications

chrome/browser/push_messaging/push_messaging_notification_manager.cc

Index: chrome/browser/push_messaging/push_messaging_notification_manager.cc
diff --git a/chrome/browser/push_messaging/push_messaging_notification_manager.cc b/chrome/browser/push_messaging/push_messaging_notification_manager.cc
index e80db64effe85f8adc5f90611d4f98bd21dda374..c9bc017ed3c1a7b7196da4a8702c383097dacf32 100644
--- a/chrome/browser/push_messaging/push_messaging_notification_manager.cc
+++ b/chrome/browser/push_messaging/push_messaging_notification_manager.cc
@@ -21,6 +21,7 @@
 #include "chrome/grit/generated_resources.h"
 #include "components/rappor/rappor_utils.h"
 #include "components/url_formatter/elide_url.h"
+#include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/platform_notification_context.h"
@@ -318,6 +319,7 @@ void PushMessagingNotificationManager::CheckForMissedNotification(
                  base::Bind(&PushMessagingNotificationManager::
                                 DidWriteNotificationDataIOProxy,
                             weak_factory_.GetWeakPtr(), origin,
+                            service_worker_registration_id,
                             database_data.notification_data,
                             message_handled_closure)));
 }
@@ -326,6 +328,7 @@ void PushMessagingNotificationManager::CheckForMissedNotification(
 void PushMessagingNotificationManager::DidWriteNotificationDataIOProxy(
     const base::WeakPtr<PushMessagingNotificationManager>& ui_weak_ptr,
     const GURL& origin,
+    int service_worker_registration_id,
     const PlatformNotificationData& notification_data,
     const base::Closure& message_handled_closure,
     bool success,
@@ -334,12 +337,14 @@ void PushMessagingNotificationManager::DidWriteNotificationDataIOProxy(
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&PushMessagingNotificationManager::DidWriteNotificationData,
-                 ui_weak_ptr, origin, notification_data,
-                 message_handled_closure, success, persistent_notification_id));
+                 ui_weak_ptr, origin, service_worker_registration_id,
+                 notification_data, message_handled_closure, success,
+                 persistent_notification_id));
 }
 
 void PushMessagingNotificationManager::DidWriteNotificationData(
     const GURL& origin,
+    int service_worker_registration_id,
     const PlatformNotificationData& notification_data,
     const base::Closure& message_handled_closure,
     bool success,
@@ -351,9 +356,52 @@ void PushMessagingNotificationManager::DidWriteNotificationData(
     return;
   }
 
+  // Get the service worker scope.

[Peter Beverloo, 2016/07/15 13:40:11]

The //content API does not allow us to do this (see my comment for the DEPS
file), so we have three options:

  (1) Expose the necessary functionality in the //content API;
  (2) Expose a more generic //content API for displaying a notification;
  (3) Don't do anything in this case.

We reach this point in the code when the website didn't show a notification by
themselves, despite being required to. This is an exceptional case, and
developers go through a lot of effort to prevent it. UMA agrees with these
statements :-).

As such, my suggestion would be to go for option (3)- revert the changes in this
file, and pass the origin instead of the Service Worker's scope. The consequence
would be that, in a small percentage of this already exceptional case, we'd
attribute the notification to Chrome as opposed to the WebAPK. I think that's
OK.

One final thing to consider here are users blocking notifications for the
WebAPK, but not for Chrome- it might show up after all... My gut feel is that we
should have UMA evidence for this case before doing //content API surgery.

+  scoped_refptr<content::ServiceWorkerContextWrapper> service_worker_context =
+      static_cast<content::ServiceWorkerContextWrapper*>(
+          GetStoragePartition(profile_, origin)->GetServiceWorkerContext());
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &content::ServiceWorkerContextWrapper::FindReadyRegistrationForId,

[pkotwicz, 2016/07/14 19:32:50]

Is it safe to call FindReadyRegistrationForId()? The method seems to have side
effects, notably calling
ServiceWorkerRegistration::ActivateWaitingVersionWhenReady()

I do not know what ServiceWorkerRegistration::ActivateWaitingVersionWhenReady()
does.

+          service_worker_context, service_worker_registration_id, origin,
+          base::Bind(&PushMessagingNotificationManager::
+                         FindServiceWorkerRegistrationCallbackIOProxy,
+                     weak_factory_.GetWeakPtr(), origin, notification_data,
+                     message_handled_closure, persistent_notification_id)));
+}
+
+// static
+void PushMessagingNotificationManager::
+    FindServiceWorkerRegistrationCallbackIOProxy(
+        const base::WeakPtr<PushMessagingNotificationManager>& ui_weak_ptr,
+        const GURL& origin,
+        const content::PlatformNotificationData& notification_data,
+        const base::Closure& message_handled_closure,
+        int64_t persistent_notification_id,
+        content::ServiceWorkerStatusCode service_worker_status,
+        const scoped_refptr<content::ServiceWorkerRegistration>& registration) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&PushMessagingNotificationManager::
+                     FindServiceWorkerRegistrationCallback,
+                 ui_weak_ptr, origin, notification_data,
+                 message_handled_closure, persistent_notification_id,
+                 service_worker_status, registration));
+}
+
+void PushMessagingNotificationManager::FindServiceWorkerRegistrationCallback(
+    const GURL& origin,
+    const content::PlatformNotificationData& notification_data,
+    const base::Closure& message_handled_closure,
+    int64_t persistent_notification_id,
+    content::ServiceWorkerStatusCode service_worker_status,
+    const scoped_refptr<content::ServiceWorkerRegistration>& registration) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   PlatformNotificationServiceImpl::GetInstance()->DisplayPersistentNotification(
-      profile_, persistent_notification_id, origin, notification_data,
-      NotificationResources());
+      profile_, persistent_notification_id, registration->pattern(), origin,
+      notification_data, NotificationResources());
 
   message_handled_closure.Run();
 }