ServiceWorker: Make '.ready' return a promise to be resolved with ServiceWorkerRegistration (2/3)

content/child/service_worker/web_service_worker_provider_impl.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/web_service_worker_provider_impl.h"
 
 #include "base/atomic_sequence_num.h"
 #include "base/logging.h"
 #include "content/child/child_thread.h"
 #include "content/child/service_worker/service_worker_dispatcher.h"
 #include "content/child/service_worker/service_worker_handle_reference.h"
 #include "content/child/service_worker/service_worker_provider_context.h"
+#include "content/child/service_worker/service_worker_registration_handle_reference.h"
 #include "content/child/service_worker/web_service_worker_impl.h"
+#include "content/child/service_worker/web_service_worker_registration_impl.h"
 #include "content/child/thread_safe_sender.h"
 #include "content/common/service_worker/service_worker_messages.h"
 #include "third_party/WebKit/public/platform/WebServiceWorkerProviderClient.h"
 #include "third_party/WebKit/public/platform/WebURL.h"
 
 using blink::WebURL;
 
 namespace content {
 
 WebServiceWorkerProviderImpl::WebServiceWorkerProviderImpl(
@@ skipping 16 matching lines @@
     return;
   }
 
   // TODO(kinuko): Here we could also register the current thread ID
   // on the provider context so that multiple WebServiceWorkerProviderImpl
   // (e.g. on document and on dedicated workers) can properly share
   // the single provider context across threads. (http://crbug.com/366538
   // for more context)
   GetDispatcher()->AddScriptClient(provider_id_, client);
 
-  if (context_->installing_handle_id() != kInvalidServiceWorkerHandleId) {
-    client->setInstalling(GetDispatcher()->GetServiceWorker(
-        context_->installing()->info(), false));
-  }
+  if (context_->active_handle_id() != kInvalidServiceWorkerHandleId) {
+    WebServiceWorkerRegistrationImpl* registration =
+        GetDispatcher()->GetServiceWorkerRegistration(
+            context_->registration()->info(), false);

[michaeln, 2014/09/04 00:27:23]

I know this is because things are in an intermediary state, but this is kinda
bizarre reading code. Here its assuming that if active_handle_id then there must
be a reg->info. Ok, if you say so?

[nhiroki, 2014/09/08 15:55:52]

Straightened the logic in the latest patchset. How does it look to you?

 
-  if (context_->waiting_handle_id() != kInvalidServiceWorkerHandleId) {
-    client->setWaiting(GetDispatcher()->GetServiceWorker(
-        context_->waiting()->info(), false));
-  }
+    if (context_->installing_handle_id() != kInvalidServiceWorkerHandleId) {
+      registration->SetInstalling(GetDispatcher()->GetServiceWorker(

[michaeln, 2014/09/04 00:27:23]

Registration may be a preexising object accoding to the impl, yet here we'll
overwrite whatever it may contain but only if we're over writing with non-null
values.

This is all so arbitrary over here in the renderer-side of things.

[nhiroki, 2014/09/08 15:55:52]

Updated. The latest patchset just reuses the existing registration without any
overwrites if possible.

+          context_->installing()->info(), false));
+    }
+    if (context_->waiting_handle_id() != kInvalidServiceWorkerHandleId) {
+      registration->SetWaiting(GetDispatcher()->GetServiceWorker(
+          context_->waiting()->info(), false));
+    }
+    registration->SetActive(GetDispatcher()->GetServiceWorker(
+        context_->active()->info(), false));
 
-  if (context_->active_handle_id() != kInvalidServiceWorkerHandleId) {
-    client->setActive(GetDispatcher()->GetServiceWorker(
-        context_->active()->info(), false));
+    client->setReady(GetDispatcher()->GetServiceWorkerRegistration(
+        context_->registration()->info(), false));

[michaeln, 2014/09/04 00:27:23]

Is there a reason to call GetServiceWorkerRegistration() again or could you use
the |registration| local variable? Sounds like that may be a rhetorical
question, but its not, the client side of this makes no sense to me.

[nhiroki, 2014/09/08 15:55:52]

Oops... that's just my mistake. The local variable must be ok. Sorry for making
you confused.

   }
 
   if (context_->controller_handle_id() != kInvalidServiceWorkerHandleId) {
     client->setController(GetDispatcher()->GetServiceWorker(
         context_->controller()->info(), false));
   }
 }
 
 void WebServiceWorkerProviderImpl::registerServiceWorker(
     const WebURL& pattern,
@@ skipping 18 matching lines @@
   if (dispatcher)
     dispatcher->RemoveScriptClient(provider_id_);
 }
 
 ServiceWorkerDispatcher* WebServiceWorkerProviderImpl::GetDispatcher() {
   return ServiceWorkerDispatcher::GetOrCreateThreadSpecificInstance(
       thread_safe_sender_.get());
 }
 
 }  // namespace content