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

content/browser/service_worker/service_worker_controllee_request_handler.cc

Index: content/browser/service_worker/service_worker_controllee_request_handler.cc
diff --git a/content/browser/service_worker/service_worker_controllee_request_handler.cc b/content/browser/service_worker/service_worker_controllee_request_handler.cc
index 0db1289002c8db8b2ab01d41aa5a8888cbb0da06..f19a52c6736ba9cf4901a9aa6f0c14c47e338551 100644
--- a/content/browser/service_worker/service_worker_controllee_request_handler.cc
+++ b/content/browser/service_worker/service_worker_controllee_request_handler.cc
@@ -139,6 +139,7 @@ ServiceWorkerControlleeRequestHandler::DidLookupRegistrationForMainResource(
     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();
 
@@ -182,7 +183,7 @@ ServiceWorkerControlleeRequestHandler::DidLookupRegistrationForMainResource(
     return;
   }
 
-  provider_host_->AssociateRegistration(registration.get());
+  provider_host_->SetControllerVersionAttribute(active_version.get());
   job_->ForwardToServiceWorker();
   TRACE_EVENT_ASYNC_END2(
       "ServiceWorker",
@@ -202,7 +203,7 @@ void ServiceWorkerControlleeRequestHandler::OnVersionStatusChanged(
     return;
   }
 
-  provider_host_->AssociateRegistration(registration);
+  provider_host_->SetControllerVersionAttribute(registration->active_version());
   job_->ForwardToServiceWorker();
 }