Flush out initial [un]registerServiceWorker roundtrip

content/child/service_worker/service_worker_dispatcher.cc

+// Copyright 2013 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/child/service_worker/service_worker_dispatcher.h"
+
+#include "base/lazy_instance.h"
+#include "base/threading/thread_local.h"
+#include "content/child/service_worker/service_worker_message_filter.h"
+#include "content/child/service_worker/web_service_worker_proxy.h"
+#include "content/child/thread_safe_sender.h"
+#include "content/common/service_worker_messages.h"
+
+using WebKit::WebServiceWorkerRegistry;
+using base::ThreadLocalPointer;
+using webkit_glue::WorkerTaskRunner;
+
+namespace content {
+
+static base::LazyInstance<ThreadLocalPointer<ServiceWorkerDispatcher> >::Leaky
+    g_dispatcher_tls = LAZY_INSTANCE_INITIALIZER;
+
+ServiceWorkerDispatcher* const kHasBeenDeleted =
+    reinterpret_cast<ServiceWorkerDispatcher*>(0x1);
+
+ServiceWorkerDispatcher::ServiceWorkerDispatcher(
+    ThreadSafeSender* thread_safe_sender,
+    ServiceWorkerMessageFilter* message_filter)
+    : thread_safe_sender_(thread_safe_sender), message_filter_(message_filter) {
+  g_dispatcher_tls.Pointer()->Set(this);
+}
+
+ServiceWorkerDispatcher::~ServiceWorkerDispatcher() {
+  g_dispatcher_tls.Pointer()->Set(kHasBeenDeleted);
+}
+
+void ServiceWorkerDispatcher::OnMessageReceived(const IPC::Message& msg) {
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(ServiceWorkerDispatcher, msg)
+    IPC_MESSAGE_HANDLER(ServiceWorkerMsg_ServiceWorkerRegistered,
+                        OnServiceWorkerRegistered)
+    IPC_MESSAGE_HANDLER(ServiceWorkerMsg_ServiceWorkerUnregistered,
+                        OnServiceWorkerUnregistered)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+    IPC_END_MESSAGE_MAP()
+  DCHECK(handled) << "Unhandled message:" << msg.type();
+}
+
+bool ServiceWorkerDispatcher::Send(IPC::Message* msg) {
+  return thread_safe_sender_->Send(msg);
+}
+
+static int CurrentWorkerId() {
+  return WorkerTaskRunner::Instance()->CurrentWorkerId();
+}
+
+void ServiceWorkerDispatcher::RegisterServiceWorker(
+    const string16& pattern,
+    const GURL& scriptUrl,
+    WebServiceWorkerRegistry::WebServiceWorkerCallbacks* callbacks) {
+  DCHECK(callbacks);
+  int request_id = pending_callbacks_.Add(callbacks);
+  message_filter_->RegisterRequestID(request_id, CurrentWorkerId());
+  thread_safe_sender_->Send(new ServiceWorkerHostMsg_RegisterServiceWorker(
+      request_id, pattern, scriptUrl));
+}
+
+void ServiceWorkerDispatcher::UnregisterServiceWorker(
+    const string16& pattern,
+    WebServiceWorkerRegistry::WebServiceWorkerCallbacks* callbacks) {
+  DCHECK(callbacks);
+  int request_id = pending_callbacks_.Add(callbacks);
+  message_filter_->RegisterRequestID(request_id, CurrentWorkerId());

[michaeln, 2013/10/04 01:26:07]

Looks like two different dispatchers can generate the same request_id and which
will collide in the filters map, request_id needs to be a global monotonically
increasing value for this to work.

[michaeln, 2013/10/04 01:51:52]

In case it's not obvious, i have a preference for sending the curworkerid in
messages for the round trip :) Rational is its simpler and less error prone
code, please look at the two bugs i've pointed out with the map maintenance
stuff in this review as real world evidence of that (and bonus, no locks or
extra maps).

[kinuko, 2013/10/04 14:55:48]

:( I have to agree that map stuff has some error-prone nature while it isolates
thread-related details to one process.

Either way works for me anyway, feel free to change this to the other pattern
(or my preference is not to implement these curworkerid handlings until we
really need it)

[alecflett, 2013/10/07 18:33:58]

oh wow yeah that means the quota api has this problem today?

[alecflett, 2013/10/07 18:33:58]

Given the choice of using the global vs passing the extra value, I'll go with
the extra value.

I'll just rip out the map.

+  thread_safe_sender_->Send(
+      new ServiceWorkerHostMsg_UnregisterServiceWorker(request_id, pattern));
+}
+
+ServiceWorkerDispatcher* ServiceWorkerDispatcher::ThreadSpecificInstance(
+    ThreadSafeSender* thread_safe_sender,
+    ServiceWorkerMessageFilter* message_filter) {
+  if (g_dispatcher_tls.Pointer()->Get() == kHasBeenDeleted) {
+    NOTREACHED() << "Re-instantiating TLS ServiceWorkerDispatcher.";
+    g_dispatcher_tls.Pointer()->Set(NULL);
+  }
+  if (g_dispatcher_tls.Pointer()->Get())
+    return g_dispatcher_tls.Pointer()->Get();
+
+  ServiceWorkerDispatcher* dispatcher =
+      new ServiceWorkerDispatcher(thread_safe_sender, message_filter);
+  if (WorkerTaskRunner::Instance()->CurrentWorkerId())
+    webkit_glue::WorkerTaskRunner::Instance()->AddStopObserver(dispatcher);
+  return dispatcher;
+}
+
+void ServiceWorkerDispatcher::OnServiceWorkerRegistered(
+    int32 request_id,
+    int32 service_worker_id) {
+  WebServiceWorkerRegistry::WebServiceWorkerCallbacks* callbacks =
+      pending_callbacks_.Lookup(request_id);
+  DCHECK(callbacks);
+  if (!callbacks)
+    return;
+
+  // the browser has to generate the service_worker_id so the same
+  // worker can be called from different renderer contexts. However,
+  // the proxy object doesn't have to be consistent unless we require
+  // the DOM objects to be identical when there's a duplicate
+  // registration. So for now we mint a new object each time.
+  WebServiceWorkerImpl* worker =
+      new WebServiceWorkerImpl(service_worker_id, true);
+  callbacks->onSuccess(worker);

[kinuko, 2013/10/04 14:55:48]

pending_callbacks_.Remove(request_id) here?

Current SW blink API looks to pass several raw ptrs between module boundaries,
it'd be nice to have some comments in the API header file to note if a given
pointer needs to be freed by the callee or not.

+}
+
+void ServiceWorkerDispatcher::OnServiceWorkerUnregistered(
+    int32 request_id,
+    int32 service_worker_id) {
+  WebServiceWorkerRegistry::WebServiceWorkerCallbacks* callbacks =
+      pending_callbacks_.Lookup(request_id);
+  DCHECK(callbacks);
+  if (!callbacks)
+    return;
+
+  // see notes in ServiceWorkerRegistered
+  WebServiceWorkerImpl* worker =
+      new WebServiceWorkerImpl(service_worker_id, false);
+  callbacks->onSuccess(worker);
+}
+
+void ServiceWorkerDispatcher::OnWorkerRunLoopStopped() {
+  message_filter_->ClearThreadRequests(CurrentWorkerId());
+  delete this;
+}
+
+}  // namespace content