Convert push_messaging IPC msgs into mojo interfaces

content/child/push_messaging/push_provider.cc

Index: content/child/push_messaging/push_provider.cc
diff --git a/content/child/push_messaging/push_provider.cc b/content/child/push_messaging/push_provider.cc
index 487af173e45877da0d302676932642516f121ac1..05b581559f8b234477a322599cac388770b8d93f 100644
--- a/content/child/push_messaging/push_provider.cc
+++ b/content/child/push_messaging/push_provider.cc
@@ -7,16 +7,15 @@
 #include <memory>
 #include <utility>
 
+#include "base/bind_helpers.h"
 #include "base/lazy_instance.h"
 #include "base/memory/ptr_util.h"
-#include "base/stl_util.h"
 #include "base/threading/thread_local.h"
-#include "content/child/push_messaging/push_dispatcher.h"
+#include "content/child/child_thread_impl.h"
 #include "content/child/service_worker/web_service_worker_registration_impl.h"
-#include "content/child/thread_safe_sender.h"
-#include "content/common/push_messaging_messages.h"
 #include "content/public/common/child_process_host.h"
 #include "content/public/common/push_subscription_options.h"
+#include "services/service_manager/public/cpp/interface_provider.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/platform/modules/push_messaging/WebPushSubscription.h"
 #include "third_party/WebKit/public/platform/modules/push_messaging/WebPushSubscriptionOptions.h"
@@ -73,10 +72,16 @@ blink::WebPushError PushRegistrationStatusToWebPushError(
 static base::LazyInstance<base::ThreadLocalPointer<PushProvider>>::Leaky
     g_push_provider_tls = LAZY_INSTANCE_INITIALIZER;
 
-PushProvider::PushProvider(ThreadSafeSender* thread_safe_sender,
-                           PushDispatcher* push_dispatcher)
-    : thread_safe_sender_(thread_safe_sender),
-      push_dispatcher_(push_dispatcher) {
+PushProvider::PushProvider(
+    scoped_refptr<base::SingleThreadTaskRunner>& main_thread_task_runner) {
+  DCHECK(main_thread_task_runner.get());

[Peter Beverloo, 2017/02/16 16:08:32]

micro nit: I don't think we need .get() here

[ke.he, 2017/02/17 08:22:30]

Done.

+
+  if (main_thread_task_runner) {

[Peter Beverloo, 2017/02/16 16:08:32]

Should this be something like the following? We now know that
|main_thread_task_runner| is always set, so we can avoid this if, but we can
also avoid posting a task if we're already on the main thread.

if (!main_thread_task_runner->BelongsToCurrentThread()) {
  mttr->PostTask(PP:BindRequest, base::Passed(&request))
} else {
  BindRequest(std::move(request))
}

[ke.he, 2017/02/17 08:22:29]

Yeah, no need to postTask to itself.
Done :)

+    auto request = mojo::MakeRequest(&push_messaging_);
+    main_thread_task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(&PushProvider::BindRequest, base::Passed(&request)));
+  }
   g_push_provider_tls.Pointer()->Set(this);
 }
 
@@ -85,18 +90,23 @@ PushProvider::~PushProvider() {
 }
 
 PushProvider* PushProvider::ThreadSpecificInstance(
-    ThreadSafeSender* thread_safe_sender,
-    PushDispatcher* push_dispatcher) {
+    scoped_refptr<base::SingleThreadTaskRunner>& main_thread_task_runner) {
   if (g_push_provider_tls.Pointer()->Get())
     return g_push_provider_tls.Pointer()->Get();
 
-  PushProvider* provider =
-      new PushProvider(thread_safe_sender, push_dispatcher);
+  PushProvider* provider = new PushProvider(main_thread_task_runner);
   if (CurrentWorkerId())
     WorkerThread::AddObserver(provider);
   return provider;
 }
 
+// static
+void PushProvider::BindRequest(mojom::PushMessagingRequest request) {

[Peter Beverloo, 2017/02/16 16:08:32]

nit: could we call this GetInterface? It's not really binding the request, which
is a concept that Mojo uses if this provider were to implement a client as well.
Sorry that I missed this in the previous round!

[ke.he, 2017/02/17 08:22:29]

Done.

+  if (ChildThreadImpl::current())

[Peter Beverloo, 2017/02/16 16:08:32]

nit: {}

[ke.he, 2017/02/17 08:22:29]

Done.

+    ChildThreadImpl::current()->GetRemoteInterfaces()->GetInterface(
+        std::move(request));
+}
+
 void PushProvider::WillStopCurrentWorkerThread() {
   delete this;
 }
@@ -107,8 +117,7 @@ void PushProvider::subscribe(
     std::unique_ptr<blink::WebPushSubscriptionCallbacks> callbacks) {
   DCHECK(service_worker_registration);
   DCHECK(callbacks);
-  int request_id = push_dispatcher_->GenerateRequestId(CurrentWorkerId());
-  subscription_callbacks_.AddWithID(std::move(callbacks), request_id);
+
   int64_t service_worker_registration_id =
       GetServiceWorkerRegistrationId(service_worker_registration);
   PushSubscriptionOptions content_options;
@@ -117,9 +126,40 @@ void PushProvider::subscribe(
   // Just treat the server key as a string of bytes and pass it to the push
   // service.
   content_options.sender_info = options.applicationServerKey.latin1();
-  thread_safe_sender_->Send(new PushMessagingHostMsg_Subscribe(
-      ChildProcessHost::kInvalidUniqueID, request_id,
-      service_worker_registration_id, content_options));
+  if (push_messaging_) {

[Peter Beverloo, 2017/02/16 16:08:32]

Can we drop these if()s everywhere?

If we can't immediately because of test failures, we should figure out a path
towards fixing the test failures. Tests should not end up using this class if
the backing infrastructure hasn't been provided.

[ke.he, 2017/02/17 08:22:29]

We can drop the if(). I found |if(push_messaging_)| always returns true because
now the MakeRequest() always be called in constructor.

And I guess it won't break any test, I'll check the trybot after cl uploaded.
Done.

+    push_messaging_->Subscribe(
+        ChildProcessHost::kInvalidUniqueID, service_worker_registration_id,
+        content_options,
+        base::Bind(&PushProvider::SubscribeCallback, base::Unretained(this),

[Peter Beverloo, 2017/02/16 16:08:32]

Is it safe to use base::Unretained because |push_messaging_| is owned by |this|?
We try to always add a comment w/ justification when using base::Unretained.

[ke.he, 2017/02/17 08:22:30]

Comments are added when using base::Unretained.
Done.

+                   base::Passed(&callbacks)));
+  }
+}
+
+void PushProvider::SubscribeCallback(
+    std::unique_ptr<blink::WebPushSubscriptionCallbacks> callbacks,
+    content::PushRegistrationStatus status,
+    const base::Optional<GURL>& endpoint,
+    const base::Optional<content::PushSubscriptionOptions>& options,
+    const base::Optional<std::vector<uint8_t>>& p256dh,
+    const base::Optional<std::vector<uint8_t>>& auth) {
+  if (!callbacks) {
+    return;
+  }
+
+  if (status == PUSH_REGISTRATION_STATUS_SUCCESS_FROM_PUSH_SERVICE ||
+      status == PUSH_REGISTRATION_STATUS_SUCCESS_FROM_CACHE) {
+    DCHECK(endpoint != base::nullopt);
+    DCHECK(options != base::nullopt);
+    DCHECK(p256dh != base::nullopt);
+    DCHECK(auth != base::nullopt);
+
+    callbacks->onSuccess(base::MakeUnique<blink::WebPushSubscription>(
+        endpoint.value(), options.value().user_visible_only,
+        blink::WebString::fromLatin1(options.value().sender_info),
+        p256dh.value(), auth.value()));
+  } else {
+    callbacks->onError(PushRegistrationStatusToWebPushError(status));
+  }
 }
 
 void PushProvider::unsubscribe(
@@ -128,12 +168,33 @@ void PushProvider::unsubscribe(
   DCHECK(service_worker_registration);
   DCHECK(callbacks);
 
-  int request_id = push_dispatcher_->GenerateRequestId(CurrentWorkerId());
-  unsubscribe_callbacks_.AddWithID(std::move(callbacks), request_id);
   int64_t service_worker_registration_id =
       GetServiceWorkerRegistrationId(service_worker_registration);
-  thread_safe_sender_->Send(new PushMessagingHostMsg_Unsubscribe(
-      request_id, service_worker_registration_id));
+
+  if (push_messaging_) {
+    push_messaging_->Unsubscribe(
+        service_worker_registration_id,
+        base::Bind(&PushProvider::UnsubscribeCallback, base::Unretained(this),
+                   base::Passed(&callbacks)));
+  }
+}
+
+void PushProvider::UnsubscribeCallback(
+    std::unique_ptr<blink::WebPushUnsubscribeCallbacks> callbacks,
+    bool is_success,
+    bool did_unsubscribe,
+    blink::WebPushError::ErrorType error_type,
+    const base::Optional<std::string>& error_message) {
+  if (!callbacks) {
+    return;
+  }
+
+  if (is_success) {
+    callbacks->onSuccess(did_unsubscribe);
+  } else {
+    callbacks->onError(blink::WebPushError(
+        error_type, blink::WebString::fromUTF8(error_message->c_str())));
+  }
 }
 
 void PushProvider::getSubscription(
@@ -141,170 +202,81 @@ void PushProvider::getSubscription(
     std::unique_ptr<blink::WebPushSubscriptionCallbacks> callbacks) {
   DCHECK(service_worker_registration);
   DCHECK(callbacks);
-  int request_id = push_dispatcher_->GenerateRequestId(CurrentWorkerId());
-  subscription_callbacks_.AddWithID(std::move(callbacks), request_id);
-  int64_t service_worker_registration_id =
-      GetServiceWorkerRegistrationId(service_worker_registration);
-  thread_safe_sender_->Send(new PushMessagingHostMsg_GetSubscription(
-      request_id, service_worker_registration_id));
-}
 
-void PushProvider::getPermissionStatus(
-    blink::WebServiceWorkerRegistration* service_worker_registration,
-    const blink::WebPushSubscriptionOptions& options,
-    std::unique_ptr<blink::WebPushPermissionStatusCallbacks> callbacks) {
-  DCHECK(service_worker_registration);
-  DCHECK(callbacks);
-  int request_id = push_dispatcher_->GenerateRequestId(CurrentWorkerId());
-  permission_status_callbacks_.AddWithID(std::move(callbacks), request_id);
   int64_t service_worker_registration_id =
       GetServiceWorkerRegistrationId(service_worker_registration);
-  thread_safe_sender_->Send(new PushMessagingHostMsg_GetPermissionStatus(
-      request_id, service_worker_registration_id, options.userVisibleOnly));
-}
 
-bool PushProvider::OnMessageReceived(const IPC::Message& message) {
-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(PushProvider, message)
-    IPC_MESSAGE_HANDLER(PushMessagingMsg_SubscribeFromWorkerSuccess,
-                        OnSubscribeFromWorkerSuccess);
-    IPC_MESSAGE_HANDLER(PushMessagingMsg_SubscribeFromWorkerError,
-                        OnSubscribeFromWorkerError);
-    IPC_MESSAGE_HANDLER(PushMessagingMsg_UnsubscribeSuccess,
-                        OnUnsubscribeSuccess);
-    IPC_MESSAGE_HANDLER(PushMessagingMsg_UnsubscribeError, OnUnsubscribeError);
-    IPC_MESSAGE_HANDLER(PushMessagingMsg_GetSubscriptionSuccess,
-                        OnGetSubscriptionSuccess);
-    IPC_MESSAGE_HANDLER(PushMessagingMsg_GetSubscriptionError,
-                        OnGetSubscriptionError);
-    IPC_MESSAGE_HANDLER(PushMessagingMsg_GetPermissionStatusSuccess,
-                        OnGetPermissionStatusSuccess);
-    IPC_MESSAGE_HANDLER(PushMessagingMsg_GetPermissionStatusError,
-                        OnGetPermissionStatusError);
-    IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-
-  return handled;
-}
-
-void PushProvider::OnSubscribeFromWorkerSuccess(
-    int request_id,
-    const GURL& endpoint,
-    const PushSubscriptionOptions& options,
-    const std::vector<uint8_t>& p256dh,
-    const std::vector<uint8_t>& auth) {
-  blink::WebPushSubscriptionCallbacks* callbacks =
-      subscription_callbacks_.Lookup(request_id);
-  if (!callbacks)
-    return;
-
-  callbacks->onSuccess(base::MakeUnique<blink::WebPushSubscription>(
-      endpoint, options.user_visible_only,
-      blink::WebString::fromLatin1(options.sender_info), p256dh, auth));
-
-  subscription_callbacks_.Remove(request_id);
-}
-
-void PushProvider::OnSubscribeFromWorkerError(int request_id,
-                                              PushRegistrationStatus status) {
-  blink::WebPushSubscriptionCallbacks* callbacks =
-      subscription_callbacks_.Lookup(request_id);
-  if (!callbacks)
-    return;
-
-  callbacks->onError(PushRegistrationStatusToWebPushError(status));
-
-  subscription_callbacks_.Remove(request_id);
-}
-
-void PushProvider::OnUnsubscribeSuccess(int request_id, bool did_unsubscribe) {
-  blink::WebPushUnsubscribeCallbacks* callbacks =
-      unsubscribe_callbacks_.Lookup(request_id);
-  if (!callbacks)
-    return;
-
-  callbacks->onSuccess(did_unsubscribe);
-
-  unsubscribe_callbacks_.Remove(request_id);
-}
-
-void PushProvider::OnUnsubscribeError(int request_id,
-                                      blink::WebPushError::ErrorType error_type,
-                                      const std::string& error_message) {
-  blink::WebPushUnsubscribeCallbacks* callbacks =
-      unsubscribe_callbacks_.Lookup(request_id);
-  if (!callbacks)
-    return;
-
-  callbacks->onError(blink::WebPushError(
-      error_type, blink::WebString::fromUTF8(error_message)));
-
-  unsubscribe_callbacks_.Remove(request_id);
-}
-
-void PushProvider::OnGetSubscriptionSuccess(
-    int request_id,
-    const GURL& endpoint,
-    const PushSubscriptionOptions& options,
-    const std::vector<uint8_t>& p256dh,
-    const std::vector<uint8_t>& auth) {
-  blink::WebPushSubscriptionCallbacks* callbacks =
-      subscription_callbacks_.Lookup(request_id);
-  if (!callbacks)
-    return;
-
-  callbacks->onSuccess(base::MakeUnique<blink::WebPushSubscription>(
-      endpoint, options.user_visible_only,
-      blink::WebString::fromLatin1(options.sender_info), p256dh, auth));
-
-  subscription_callbacks_.Remove(request_id);
+  if (push_messaging_) {
+    push_messaging_->GetSubscription(
+        service_worker_registration_id,
+        base::Bind(&PushProvider::GetSubscriptionCallback,
+                   base::Unretained(this), base::Passed(&callbacks)));
+  }
 }
 
-void PushProvider::OnGetSubscriptionError(int request_id,
-                                          PushGetRegistrationStatus status) {
-  blink::WebPushSubscriptionCallbacks* callbacks =
-      subscription_callbacks_.Lookup(request_id);
+void PushProvider::GetSubscriptionCallback(
+    std::unique_ptr<blink::WebPushSubscriptionCallbacks> callbacks,
+    content::PushGetRegistrationStatus status,
+    const base::Optional<GURL>& endpoint,
+    const base::Optional<content::PushSubscriptionOptions>& options,
+    const base::Optional<std::vector<uint8_t>>& p256dh,
+    const base::Optional<std::vector<uint8_t>>& auth) {
   if (!callbacks)
     return;

[Peter Beverloo, 2017/02/16 16:08:32]

When would this be NULL (elsewhere too)? Can we DCHECK?

(I see that we already had these null checks, while we shouldn't have. Happy to
fix this in a follow-up.)

[ke.he, 2017/02/17 08:22:33]

I think DCHECK should be fine here. Anyway Let's fix it in a follow-up.
Tracked by 693366.

 
-  // We are only expecting an error if we can't find a registration.
-  callbacks->onSuccess(nullptr);
-
-  subscription_callbacks_.Remove(request_id);
+  if (status == PUSH_GETREGISTRATION_STATUS_SUCCESS) {
+    DCHECK(endpoint != base::nullopt);
+    DCHECK(options != base::nullopt);
+    DCHECK(p256dh != base::nullopt);
+    DCHECK(auth != base::nullopt);

[Peter Beverloo, 2017/02/16 16:08:32]

nit: does DCHECK_NE work here?

[ke.he, 2017/02/17 08:22:32]

compiling failed on DCHECK_NE here.

In optional.md it says: "When initialized without a value, `base::Optional<T>`
will be empty. When empty, the `operator bool` will return `false` ... An empty
`base::Optional<T>` is equal to `base::nullopt_t`."
"

So I think DCHECK(endpoint) is good.

+
+    callbacks->onSuccess(base::MakeUnique<blink::WebPushSubscription>(
+        endpoint.value(), options.value().user_visible_only,
+        blink::WebString::fromLatin1(options.value().sender_info),
+        p256dh.value(), auth.value()));
+  } else {
+    // We are only expecting an error if we can't find a registration.
+    callbacks->onSuccess(nullptr);
+  }
 }
 
-void PushProvider::OnGetPermissionStatusSuccess(
-    int request_id,
-    blink::WebPushPermissionStatus status) {
-  blink::WebPushPermissionStatusCallbacks* callbacks =
-      permission_status_callbacks_.Lookup(request_id);
-  if (!callbacks)
-    return;
-
-  callbacks->onSuccess(status);
+void PushProvider::getPermissionStatus(
+    blink::WebServiceWorkerRegistration* service_worker_registration,
+    const blink::WebPushSubscriptionOptions& options,
+    std::unique_ptr<blink::WebPushPermissionStatusCallbacks> callbacks) {
+  DCHECK(service_worker_registration);
+  DCHECK(callbacks);
 
-  permission_status_callbacks_.Remove(request_id);
+  int64_t service_worker_registration_id =
+      GetServiceWorkerRegistrationId(service_worker_registration);
+  if (push_messaging_) {
+    push_messaging_->GetPermissionStatus(
+        service_worker_registration_id, options.userVisibleOnly,
+        base::Bind(&PushProvider::GetPermissionStatusCallback,
+                   base::Unretained(this), base::Passed(&callbacks)));
+  }
 }
 
-void PushProvider::OnGetPermissionStatusError(
-    int request_id,
+void PushProvider::GetPermissionStatusCallback(
+    std::unique_ptr<blink::WebPushPermissionStatusCallbacks> callbacks,
+    bool is_success,
+    blink::WebPushPermissionStatus status,
     blink::WebPushError::ErrorType error) {
-  blink::WebPushPermissionStatusCallbacks* callbacks =
-      permission_status_callbacks_.Lookup(request_id);
   if (!callbacks)
     return;
 
-  std::string error_message;
-  if (error == blink::WebPushError::ErrorTypeNotSupported) {
-    error_message =
-        "Push subscriptions that don't enable userVisibleOnly are not "
-        "supported.";
+  if (is_success) {
+    callbacks->onSuccess(status);
+  } else {
+    std::string error_message;
+    if (error == blink::WebPushError::ErrorTypeNotSupported) {
+      error_message =
+          "Push subscriptions that don't enable userVisibleOnly are not "
+          "supported.";
+    }
+    callbacks->onError(
+        blink::WebPushError(error, blink::WebString::fromUTF8(error_message)));
   }
-
-  callbacks->onError(
-      blink::WebPushError(error, blink::WebString::fromUTF8(error_message)));
-
-  permission_status_callbacks_.Remove(request_id);
 }
 
 }  // namespace content