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

content/browser/service_worker/service_worker_controllee_request_handler.cc

 // Copyright 2014 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_controllee_request_handler.h"
 
 #include "base/debug/trace_event.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_metrics.h"
 #include "content/browser/service_worker/service_worker_provider_host.h"
@@ skipping 121 matching lines @@
   if (status != SERVICE_WORKER_OK) {
     job_->FallbackToNetwork();
     TRACE_EVENT_ASYNC_END1(
         "ServiceWorker",
         "ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
         job_.get(),
         "Status", status);
     return;
   }
   DCHECK(registration.get());
+  provider_host_->AssociateRegistration(registration.get());

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

To associate a potential controllee (ie. |provider_host_|) with the
registration, I separated SetControllerVersionAttribute() from
AssociateRegistration().

[michaeln, 2014/09/09 02:22:01]

Does this have to happen prior to the pre-existing AssociateRegistration
callsites below? Why is this separation needed? Generally, the client-side
doesn't see associations with a registration with an active version that is
still activating. With the early call to AssocReg here, that condition now
exists in the client-side which opens the door to corner cases that maybe don't
need to be there. So if possible i'd like to keep the regassocation and
controllingversion setting as close together as possible.

Are you concerned about the other call to AssociateProviderHostsToRegistration
in the UpdateJob class inadvertently setting the controlling version when it
shouldn't? If so, thats not a concern that callsite is only used for newly
created registrations which do not have an active version.

Are you concerned about the raciness between main doc loading settling on an
association and an update job creating a new registration? If so, i think i see
a bug with that regardless of this earlier call to AssociateRegistration(). I'll
upload a patch for it (has to do with having the docurl being set in advance of
a reg getting associated).

Would it make sense to keep them as separate methods but to invoke them on
consecutive lines in the .cc file?

[nhiroki, 2014/09/09 08:49:08]

Hmmm... I think the condition would have already existed.
SWRegisterJob::RegisterAndContinue associates hosts with a newly created
registration and then the registration takes an active version whose state is
'activating'. I'm not really sure why we don't have to associate unactivated
registration here like the register job... (or is the register job wrong??)
No, I'm not concerned about it.
Do you mean that the existing callsite of AssociateProviderHostsToRegistration
(ie. SWRegisterJob::RegisterAndContinue) is wrong 
? If so, I think this is just what I'm concerned. As I commented above, I was
confused why we don't have to associate unactivated registration during the main
loading, whereas we do so in the update job. I guess
ServiceWorkerRegistration::OnActivateEventFinished or a subsequent function is
the correct callsite... is this right?
If my previous inline comment is correct, it makes sense.

[nhiroki, 2014/09/09 11:48:35]

As per the spec discussion
(https://github.com/slightlyoff/ServiceWorker/issues/357#issuecomment-53397325),
an active worker of the ready registration could be in the activating state, so
ServiceWorkerRegistration::ActivateWaitingVersion could be better place to
associate...?

[michaeln, 2014/09/09 23:30:33]

Ah, I think your right and it definitely will have to exist when we get around
to implementing swap() too.

 
   ServiceWorkerMetrics::CountControlledPageLoad();
 
   // Initiate activation of a waiting version.
   // Usually a register job initiates activation but that
   // doesn't happen if the browser exits prior to activation
   // having occurred. This check handles that case.
   if (registration->waiting_version())
     registration->ActivateWaitingVersionWhenReady();
 
@@ skipping 23 matching lines @@
     TRACE_EVENT_ASYNC_END2(
         "ServiceWorker",
         "ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
         job_.get(),
         "Status", status,
         "Info",
         "ServiceWorkerVersion is not available, so falling back to network");
     return;
   }
 
-  provider_host_->AssociateRegistration(registration.get());
+  provider_host_->SetControllerVersionAttribute(active_version.get());
   job_->ForwardToServiceWorker();
   TRACE_EVENT_ASYNC_END2(
       "ServiceWorker",
       "ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
       job_.get(),
       "Status", status,
       "Info",
       "Forwarded to the ServiceWorker");
 }
 
 void ServiceWorkerControlleeRequestHandler::OnVersionStatusChanged(
     ServiceWorkerRegistration* registration,
     ServiceWorkerVersion* version) {
   if (version != registration->active_version() ||
       version->status() != ServiceWorkerVersion::ACTIVATED) {
     job_->FallbackToNetwork();
     return;
   }
 
-  provider_host_->AssociateRegistration(registration);
+  provider_host_->SetControllerVersionAttribute(registration->active_version());
   job_->ForwardToServiceWorker();
 }
 
 void ServiceWorkerControlleeRequestHandler::PrepareForSubResource() {
   DCHECK(job_.get());
   DCHECK(context_);
   DCHECK(provider_host_->active_version());
   job_->ForwardToServiceWorker();
 }
 
 }  // namespace content