Service Worker: in Unregister, wait until after the active worker no longer controls a document

content/browser/service_worker/service_worker_register_job.cc

Index: content/browser/service_worker/service_worker_register_job.cc
diff --git a/content/browser/service_worker/service_worker_register_job.cc b/content/browser/service_worker/service_worker_register_job.cc
index 9d1a83d3101eee6415372ca52400865016b61039..1aab25e0d6a1ed8f03c622af2a4dbc7fe55bbb42 100644
--- a/content/browser/service_worker/service_worker_register_job.cc
+++ b/content/browser/service_worker/service_worker_register_job.cc
@@ -83,7 +83,13 @@ void ServiceWorkerRegisterJob::Start() {
         &ServiceWorkerRegisterJob::ContinueWithUpdate,
         weak_factory_.GetWeakPtr());
   }
-  context_->storage()->FindRegistrationForPattern(pattern_, next_step);
+
+  scoped_refptr<ServiceWorkerRegistration> registration =
+      context_->storage()->GetUninstallingRegistration(pattern_);
+  if (registration)
+    RunSoon(base::Bind(next_step, SERVICE_WORKER_OK, registration));
+  else
+    context_->storage()->FindRegistrationForPattern(pattern_, next_step);
 }
 
 void ServiceWorkerRegisterJob::Abort() {
@@ -163,53 +169,48 @@ void ServiceWorkerRegisterJob::SetPhase(Phase phase) {
   phase_ = phase;
 }
 
-// This function corresponds to the steps in Register following
-// "Let serviceWorkerRegistration be _GetRegistration(scope)"
-// |existing_registration| corresponds to serviceWorkerRegistration.
+// This function corresponds to the steps in [[Register]] following
+// "Let registration be the result of running the [[GetRegistration]] algorithm.
 // Throughout this file, comments in quotes are excerpts from the spec.
 void ServiceWorkerRegisterJob::ContinueWithRegistration(
     ServiceWorkerStatusCode status,
     const scoped_refptr<ServiceWorkerRegistration>& existing_registration) {
   DCHECK_EQ(REGISTRATION_JOB, job_type_);
-  // On unexpected error, abort this registration job.
   if (status != SERVICE_WORKER_ERROR_NOT_FOUND && status != SERVICE_WORKER_OK) {
     Complete(status);
     return;
   }
 
-  // "If serviceWorkerRegistration is not null and script is equal to
-  // serviceWorkerRegistration.scriptUrl..." resolve with the existing
-  // registration and abort.
-  if (existing_registration.get() &&
-      existing_registration->script_url() == script_url_) {
+  if (!existing_registration) {
+    RegisterAndContinue(SERVICE_WORKER_OK);
+    return;
+  }
+
+  // "1. Set registration.[[Uninstalling]] to false."

[nhiroki, 2014/08/11 12:24:51]

"1. Set ..." -> "2. Set ..."

+  existing_registration->AbortPendingClear();
+
+  // "2. If scriptURL is equal to registration.[[ScriptURL]], then:"

[nhiroki, 2014/08/11 12:24:51]

"1. If scriptURL" would be correct.

This has the different item level from the previous one. To avoid confusion, how
about removing the number, or adding parents number? (eg. "8.1. If scriptURL
...") Wdyt?

[falken, 2014/08/12 09:06:06]

Good catches about the spec quotes. We're currently missing some steps which
causes the quotes to be rather chaotic and unordered. I intend to make the
function more clearly follow the spec in a future patch for
http://crbug.com/398355

For now, I've removed the numbering.

+  if (existing_registration->script_url() == script_url_) {
+    // Spec says to resolve with registration.[[GetNewestWorker]]. We come close
+    // by resolving with the active version.
     set_registration(existing_registration);
-    // If there's no active version, go ahead to Update (this isn't in the spec
-    // but seems reasonable, and without SoftUpdate implemented we can never
-    // Update otherwise).
+
     if (!existing_registration->active_version()) {
       UpdateAndContinue();
       return;
     }
+
     ResolvePromise(
         status, existing_registration, existing_registration->active_version());
     Complete(SERVICE_WORKER_OK);
     return;
   }
 
-  // "If serviceWorkerRegistration is null..." create a new registration.
-  if (!existing_registration.get()) {
-    RegisterAndContinue(SERVICE_WORKER_OK);
-    return;
-  }
-
-  // On script URL mismatch, "set serviceWorkerRegistration.scriptUrl to
-  // script." We accomplish this by deleting the existing registration and
-  // registering a new one.
-  // TODO(falken): Match the spec. We now throw away the active_version_ and
-  // waiting_version_ of the existing registration, which isn't in the spec.
+  // "Set registration.[[ScriptURL]] to scriptURL." We accomplish this by
+  // deleting the existing registration and registering a new one.
   // TODO(michaeln): Deactivate the live existing_registration object and
-  // eventually call storage->DeleteVersionResources()
-  // when it no longer has any controllees.
+  // eventually call storage->DeleteVersionResources() when it no longer has any
+  // controllees.
   context_->storage()->DeleteRegistration(
       existing_registration->id(),
       existing_registration->script_url().GetOrigin(),