[Background Sync] Use Mojo IPC to fire background sync events

content/browser/service_worker/service_worker_mojo_event_dispatcher.cc

Index: content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
diff --git a/content/browser/service_worker/service_worker_mojo_event_dispatcher.cc b/content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c69024edc51ef8a590968e5d41eb811da3d06f0f
--- /dev/null
+++ b/content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
@@ -0,0 +1,131 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/service_worker/service_worker_mojo_event_dispatcher.h"
+
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/common/service_registry.h"
+#include "third_party/mojo/src/mojo/public/cpp/bindings/interface_ptr_info.h"
+
+namespace content {
+
+namespace {
+
+// Converts mojo enum content::ServiceWorkerEventStatus values to Chromium
+// content::ServiceWorkerStatusCode values.
+// TODO(iclelland): Make these enums equivalent so that conversion can be a
+// static cast.
+ServiceWorkerStatusCode StatusCodeFromMojoStatus(
+    ServiceWorkerEventStatus status) {
+  ServiceWorkerStatusCode status_code = SERVICE_WORKER_OK;
+  if (status == SERVICE_WORKER_EVENT_STATUS_COMPLETED) {
+    status_code = SERVICE_WORKER_OK;
+  } else if (status == SERVICE_WORKER_EVENT_STATUS_REJECTED) {
+    status_code = SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED;
+  } else if (status == SERVICE_WORKER_EVENT_STATUS_ABORTED) {
+    status_code = SERVICE_WORKER_ERROR_ABORT;
+  } else {
+    NOTREACHED();
+  }
+  return status_code;
+}
+
+// Establishes a connection to a mojo service, and then unbinds the local side

[michaeln, 2015/06/24 00:12:00]

(mojo noob question) Where does the 'unbinding' happen in here?

[iclelland, 2015/06/24 14:29:30]

By calling .PassInterface(), and allowing the InterfacePtr<> to go out of scope,
the original connection is torn down, but we retain all of the information
required to reconstruct the connection on a new thread, in the form of an
InterfacePtrInfo<> struct.

+// of the connection from the UI thread, and returns an InterfacePtrInfo struct
+// that can be used to build a new InterfacePtr on a different thread.
+//
+// Failure is indicated by returning an invalid InterfacePtrInfo.
+//
+// This function must be called on the UI thread, as it accesses the service
+// registry member of the RenderProcessHost.
+template <typename MojoServiceType>
+mojo::InterfacePtrInfo<MojoServiceType> ConnectOnUIThread(
+    int render_process_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  mojo::InterfacePtr<MojoServiceType> mojo_interface_ptr;
+
+  RenderProcessHost* host = RenderProcessHost::FromID(render_process_id);
+  if (host) {

[michaeln, 2015/06/24 00:12:00]

we generally prefer early returns

if (!host)
  return mojo::InterfacePtr<MojoServiceType>();

ServiceRegistry* registry = host->GetServiceRegistry();
if (!registry)
  return mojo::InterfacePtr<MojoServiceType>();

mojo::InterfacePtr<MojoServiceType> mojo_interface_ptr;
registry->ConnectToRemoteService(mojo::GetProxy(&mojo_interface_ptr));

+    content::ServiceRegistry* registry = host->GetServiceRegistry();

[michaeln, 2015/06/24 00:12:00]

i don't think the content:: prefix is needed

+    if (registry) {
+      registry->ConnectToRemoteService(mojo::GetProxy(&mojo_interface_ptr));

[michaeln, 2015/06/24 00:12:01]

How expensive is it to 'connect' to a remote service? Just curious?

+    }
+  }
+  return mojo_interface_ptr.PassInterface();
+}
+
+}  // namespace
+
+ServiceWorkerMojoEventDispatcher::ServiceWorkerMojoEventDispatcher()
+    : weak_ptr_factory_(this) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+}
+
+ServiceWorkerMojoEventDispatcher::~ServiceWorkerMojoEventDispatcher() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+}
+
+void ServiceWorkerMojoEventDispatcher::DispatchSyncEvent(
+    int render_process_id,

[michaeln, 2015/06/24 00:12:00]

is this param needed? is there not one dispatcher per renderer?

[iclelland, 2015/06/24 14:29:30]

Yes, there should be one dispatcher per renderer (assuming that the embedded
worker in the ServiceWorkerVersion never changes), so this param shouldn't ever
change. We need to get the pid somewhere, though, so that we can look up the
RenderProcessHost. We can't do it at ServiceWorkerVersion construction time, as
the worker won't be running yet.

I'm taking a closer look at your comments on service_worker_version.cc to see if
there's another design that makes more sense.

+    int thread_id,
+    const StatusCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  if (!background_sync_client_.get()) {
+    // If we have not connected to the mojo service before, switch to the UI
+    // thread to connect, and then dispatch the event once the connection is
+    // established.
+    BrowserThread::PostTaskAndReplyWithResult(

[michaeln, 2015/06/24 00:12:00]

What if we get another DispatchSyncEvent() call prior to this async op being
complete?

[iclelland, 2015/06/24 14:29:30]

In that case, the second connection will be dropped in the
BindChannelAndDispatchSyncEvent method below. (But you already saw that)

+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&ConnectOnUIThread<BackgroundSyncServiceClient>,
+                   render_process_id),
+        base::Bind(
+            &ServiceWorkerMojoEventDispatcher::BindChannelAndDispatchSyncEvent,
+            weak_ptr_factory_.GetWeakPtr(), thread_id, callback));
+    return;
+  }
+  DispatchSyncEventInternal(thread_id, callback);
+}
+
+void ServiceWorkerMojoEventDispatcher::BindChannelAndDispatchSyncEvent(
+    int thread_id,
+    const StatusCallback& callback,
+    mojo::InterfacePtrInfo<BackgroundSyncServiceClient> mojo_interface_ptr) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (!background_sync_client_.get()) {

[michaeln, 2015/06/24 00:12:01]

I see, additional connect results are dropped here. Is it ok to have multiple
intf ptrs to the same servicetype coexist at the same time (i would think so,
but just checking)?

[iclelland, 2015/06/24 14:29:30]

Yes, that should be a safe pattern; it's a little bit of wasted effort; we could
probably use some atomic condition variable to avoid it running Connect twice if
we have to, but I'd rather do the simpler thing until it becomes an obvious
performance problem.

+    // If the connect call failed, mojo_interface_ptr will be an invalid
+    // pointer, so abort now.
+    if (!mojo_interface_ptr.is_valid()) {
+      OnEventFinished(callback, SERVICE_WORKER_EVENT_STATUS_ABORTED);
+      return;
+    }
+    background_sync_client_.Bind(mojo_interface_ptr.Pass());
+  }
+  DispatchSyncEventInternal(thread_id, callback);
+}
+
+void ServiceWorkerMojoEventDispatcher::DispatchSyncEventInternal(
+    int thread_id,
+    const StatusCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // TODO(iclelland): Replace this with the real event registration details
+  // crbug.com/482066
+  content::SyncRegistrationPtr null_event(content::SyncRegistration::New());
+
+  background_sync_client_->Sync(
+      null_event.Pass(), thread_id,
+      base::Bind(&ServiceWorkerMojoEventDispatcher::OnEventFinished,
+                 weak_ptr_factory_.GetWeakPtr(), callback));
+}
+
+void ServiceWorkerMojoEventDispatcher::OnEventFinished(
+    const StatusCallback& callback,
+    ServiceWorkerEventStatus result) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  callback.Run(StatusCodeFromMojoStatus(result));
+}
+
+}  // namespace content