Mojoify the notificationclick and notificationclose events

content/renderer/service_worker/service_worker_context_client.cc

Index: content/renderer/service_worker/service_worker_context_client.cc
diff --git a/content/renderer/service_worker/service_worker_context_client.cc b/content/renderer/service_worker/service_worker_context_client.cc
index 8a68da7a5bd5cfe85d6d9ce9cfb1a95ef7109f20..74fe7cc170aab5c293b0bfae7f558db24c60e319 100644
--- a/content/renderer/service_worker/service_worker_context_client.cc
+++ b/content/renderer/service_worker/service_worker_context_client.cc
@@ -208,6 +208,10 @@ struct ServiceWorkerContextClient::WorkerContextData {
   using SyncEventCallbacksMap = IDMap<std::unique_ptr<const SyncCallback>>;
   using PaymentRequestEventCallbacksMap =
       IDMap<std::unique_ptr<const PaymentRequestEventCallback>>;
+  using NotificationClickEventCallbacksMap =
+      IDMap<std::unique_ptr<const DispatchNotificationClickEventCallback>>;
+  using NotificationCloseEventCallbacksMap =
+      IDMap<std::unique_ptr<const DispatchNotificationCloseEventCallback>>;
   using PushEventCallbacksMap =
       IDMap<std::unique_ptr<const DispatchPushEventCallback>>;
   using FetchEventCallbacksMap = IDMap<std::unique_ptr<const FetchCallback>>;
@@ -245,6 +249,12 @@ struct ServiceWorkerContextClient::WorkerContextData {
   // Pending callbacks for Payment Request Events.
   PaymentRequestEventCallbacksMap payment_request_event_callbacks;
 
+  // Pending callbacks for Notification Click Events.
+  NotificationClickEventCallbacksMap notification_click_event_callbacks;
+
+  // Pending callbacks for Notification Close Events.
+  NotificationCloseEventCallbacksMap notification_close_event_callbacks;
+
   // Pending callbacks for Push Events.
   PushEventCallbacksMap push_event_callbacks;
 
@@ -411,10 +421,6 @@ void ServiceWorkerContextClient::OnMessageReceived(
   IPC_BEGIN_MESSAGE_MAP(ServiceWorkerContextClient, message)
     IPC_MESSAGE_HANDLER(ServiceWorkerMsg_ActivateEvent, OnActivateEvent)
     IPC_MESSAGE_HANDLER(ServiceWorkerMsg_InstallEvent, OnInstallEvent)
-    IPC_MESSAGE_HANDLER(ServiceWorkerMsg_NotificationClickEvent,
-                        OnNotificationClickEvent)
-    IPC_MESSAGE_HANDLER(ServiceWorkerMsg_NotificationCloseEvent,
-                        OnNotificationCloseEvent)
     IPC_MESSAGE_HANDLER(ServiceWorkerMsg_DidGetClient, OnDidGetClient)
     IPC_MESSAGE_HANDLER(ServiceWorkerMsg_DidGetClients, OnDidGetClients)
     IPC_MESSAGE_HANDLER(ServiceWorkerMsg_OpenWindowResponse,
@@ -578,6 +584,8 @@ void ServiceWorkerContextClient::willDestroyWorkerContext(
 
   // Aborts all the pending events callbacks.
   AbortPendingEventCallbacks(context_->sync_event_callbacks);
+  AbortPendingEventCallbacks(context_->notification_click_event_callbacks);
+  AbortPendingEventCallbacks(context_->notification_close_event_callbacks);
   AbortPendingEventCallbacks(context_->push_event_callbacks);
   AbortPendingEventCallbacks(context_->fetch_event_callbacks);
   AbortPendingEventCallbacks(context_->message_event_callbacks);
@@ -722,18 +730,32 @@ void ServiceWorkerContextClient::didHandleNotificationClickEvent(
     int request_id,
     blink::WebServiceWorkerEventResult result,
     double event_dispatch_time) {
-  Send(new ServiceWorkerHostMsg_NotificationClickEventFinished(
-      GetRoutingID(), request_id, result,
-      base::Time::FromDoubleT(event_dispatch_time)));
+  const DispatchNotificationClickEventCallback* callback =
+      context_->notification_click_event_callbacks.Lookup(request_id);
+  DCHECK(callback);
+
+  callback->Run(result == blink::WebServiceWorkerEventResultCompleted
+                    ? SERVICE_WORKER_OK
+                    : SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,

[shimazu, 2017/02/10 08:09:29]

You can use "EventResultToStatus(result)" here.

[Peter Beverloo, 2017/02/10 16:41:06]

Done.

+                base::Time::FromDoubleT(event_dispatch_time));
+
+  context_->notification_click_event_callbacks.Remove(request_id);
 }
 
 void ServiceWorkerContextClient::didHandleNotificationCloseEvent(
     int request_id,
     blink::WebServiceWorkerEventResult result,
     double event_dispatch_time) {
-  Send(new ServiceWorkerHostMsg_NotificationCloseEventFinished(
-      GetRoutingID(), request_id, result,
-      base::Time::FromDoubleT(event_dispatch_time)));
+  const DispatchNotificationCloseEventCallback* callback =
+      context_->notification_close_event_callbacks.Lookup(request_id);
+  DCHECK(callback);
+
+  callback->Run(result == blink::WebServiceWorkerEventResultCompleted
+                    ? SERVICE_WORKER_OK
+                    : SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,

[shimazu, 2017/02/10 08:09:29]

ditto

[Peter Beverloo, 2017/02/10 16:41:06]

Done.

+                base::Time::FromDoubleT(event_dispatch_time));
+
+  context_->notification_close_event_callbacks.Remove(request_id);
 }
 
 void ServiceWorkerContextClient::didHandlePushEvent(
@@ -1029,26 +1051,37 @@ void ServiceWorkerContextClient::DispatchFetchEvent(
   }
 }
 
-void ServiceWorkerContextClient::OnNotificationClickEvent(
-    int request_id,
+void ServiceWorkerContextClient::DispatchNotificationClickEvent(
     const std::string& notification_id,
     const PlatformNotificationData& notification_data,
     int action_index,
-    const base::NullableString16& reply) {
+    const base::Optional<base::string16>& reply,
+    const DispatchNotificationClickEventCallback& callback) {
   TRACE_EVENT0("ServiceWorker",
-               "ServiceWorkerContextClient::OnNotificationClickEvent");
+               "ServiceWorkerContextClient::DispatchNotificationClickEvent");
+
+  int request_id = context_->notification_click_event_callbacks.Add(
+      base::MakeUnique<DispatchNotificationClickEventCallback>(callback));
+
+  blink::WebString web_reply;
+  if (reply)
+    web_reply = blink::WebString::fromUTF16(reply.value());
+
   proxy_->dispatchNotificationClickEvent(
       request_id, blink::WebString::fromUTF8(notification_id),
-      ToWebNotificationData(notification_data), action_index,
-      blink::WebString::fromUTF16(reply));
+      ToWebNotificationData(notification_data), action_index, web_reply);

[shimazu, 2017/02/10 08:09:29]

I think you can use blink.mojom.Notification directly instead of
ToWebNotificationData. Using PlatformNotificationData is really straightforward,
but I'd like to have less [Native]s and like to remove _messages.h.
ConsoleMessage might be a good reference for type-mapping to a blink type.
https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/console_me...

Creating another patch to introduce the typemap is also possible.
WDYT?

[Peter Beverloo, 2017/02/10 16:41:06]

This relates to the layering issue that we spoke about last week.

Keeping WebNotificationData as the type used in Blink is an interesting idea.
The type map configuration would be a bit tricky due to the mojo variants;

  - We need WebNotificationData (WTF) -> PlatformNotificationData (STL) for
Blink to Chromium.
  - We need PlatformNotificationData (STL) -> WebNotificationData (STL) for
Chromium to Blink. (Since we receive the mojo call in //content.)

Then, when onion soup comes into play, we also need PlatformNotificationData
(STL) -> WebNotificationData (WTF).

I quite like that idea - thanks! I will have to spend some time figuring out
whether the Mojo bindings like having asymmetrical conversions, so I would like
to do that in a separate patch if that's OK with you.

[shimazu, 2017/02/13 00:38:56]

Ah, sorry, I didn't understand accurately what you mentioned. The point here is
NotificationDatabaseData which is a container used under browser/ contains
PlatformNotificationData, so changing the type of argument for
DispatchNotificationClickEvent() doesn't seem reasonable.
In that case, using [Native] makes sense. 

Thanks for your kind explanation! :)

 }
 
-void ServiceWorkerContextClient::OnNotificationCloseEvent(
-    int request_id,
+void ServiceWorkerContextClient::DispatchNotificationCloseEvent(
     const std::string& notification_id,
-    const PlatformNotificationData& notification_data) {
+    const PlatformNotificationData& notification_data,
+    const DispatchNotificationCloseEventCallback& callback) {
   TRACE_EVENT0("ServiceWorker",
-               "ServiceWorkerContextClient::OnNotificationCloseEvent");
+               "ServiceWorkerContextClient::DispatchNotificationCloseEvent");
+
+  int request_id = context_->notification_close_event_callbacks.Add(
+      base::MakeUnique<DispatchNotificationCloseEventCallback>(callback));
+
   proxy_->dispatchNotificationCloseEvent(
       request_id, blink::WebString::fromUTF8(notification_id),
       ToWebNotificationData(notification_data));