ServiceWorker: Add support for .skipWaiting and controllerchange event(2/3)

content/browser/service_worker/service_worker_registration.cc

Index: content/browser/service_worker/service_worker_registration.cc
diff --git a/content/browser/service_worker/service_worker_registration.cc b/content/browser/service_worker/service_worker_registration.cc
index 0445f08f7e646ba73497be4257adeb4d6ad008c3..901df96bb3bc345bbbc996439e4538b27320aa13 100644
--- a/content/browser/service_worker/service_worker_registration.cc
+++ b/content/browser/service_worker/service_worker_registration.cc
@@ -210,7 +210,6 @@ void ServiceWorkerRegistration::OnNoControllees(ServiceWorkerVersion* version) {
 void ServiceWorkerRegistration::ActivateWaitingVersion() {
   DCHECK(context_);
   DCHECK(waiting_version());
-  DCHECK(should_activate_when_ready_);
   should_activate_when_ready_ = false;
   scoped_refptr<ServiceWorkerVersion> activating_version = waiting_version();
   scoped_refptr<ServiceWorkerVersion> exiting_version = active_version();
@@ -220,6 +219,9 @@ void ServiceWorkerRegistration::ActivateWaitingVersion() {
     return;  // Activation is no longer relevant.
   }
 
+  if (activating_version->skip_waiting())
+    PrepareProviderHostsToSkipWaiting();
+
   // "4. If exitingWorker is not null,
   if (exiting_version.get()) {
     DCHECK(!exiting_version->HasControllee());
@@ -239,16 +241,41 @@ void ServiceWorkerRegistration::ActivateWaitingVersion() {
 
   // "7. Run the [[UpdateState]] algorithm passing registration.activeWorker and
   // "activating" as arguments."
+  // "8. Fire a simple event named controllerchange..."
   activating_version->SetStatus(ServiceWorkerVersion::ACTIVATING);
 
-  // TODO(nhiroki): "8. Fire a simple event named controllerchange..."
-
   // "9. Queue a task to fire an event named activate..."
   activating_version->DispatchActivateEvent(
       base::Bind(&ServiceWorkerRegistration::OnActivateEventFinished,
                  this, activating_version));
 }
 
+void ServiceWorkerRegistration::PrepareProviderHostsToSkipWaiting() {
+  for (scoped_ptr<ServiceWorkerContextCore::ProviderHostIterator> it =
+           context_->GetProviderHostIterator();
+       !it->IsAtEnd(); it->Advance()) {
+    ServiceWorkerProviderHost* host = it->GetProviderHost();
+    if (!ServiceWorkerUtils::ScopeMatches(pattern(),
+                                          host->document_url()))
+      continue;
+    scoped_refptr<ServiceWorkerVersion> exiting_version =
+        host->active_version();

[falken, 2014/11/14 04:00:15]

You can add "1. Let exitingWorker be the active worker that controls client." to
help the reader find this place in the spec.

+    // "2. If exitingWorker is not null, then:"
+    if (!exiting_version.get())
+      continue;
+    // TODO(xiang): should wait for events to be complete
+    // "1. Wait for exitingWorker to finish handling any in-progress requests."
+    // "2. Terminate exitingWorker."
+    exiting_version->StopWorker(
+        base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+    exiting_version->SetStatus(ServiceWorkerVersion::REDUNDANT);

[michaeln, 2014/11/14 02:05:11]

I don't believe this is the desired behavior. What you're calling
"exiting_version" could very well be a version from a completely different
registration. Other registrations and pages controlled by them should not be
affected.

Nothing in the steps for 4.1.7 suggests the behavior you're implementing. And
the non-normlative language sayz its purpose is to promote to "active even while
clients are using the registration".

[falken, 2014/11/14 04:00:15]

Right I think this can happen if this registration's pattern matches the
document, but another registration has a longer pattern match. So you have to
skip if existing_version's registration is not us.

I think this is a bug in the spec, which just says: "For each service worker
client client whose client url matches registration's scope url". Could you open
a spec bug?

[xiang, 2014/11/14 13:17:28]

Yes, a longer matched registration should not be affected.

However there are still cases that the new "skip-waiting" registration is a
subset of existing one. We should replace these clients' controllers even they
are controlled by different registrations. 

I have open a spec issue and made a proposal for this problem:
https://github.com/slightlyoff/ServiceWorker/issues/561

Please take a look, thanks.

[michaeln, 2014/11/14 23:06:39]

Hmmm, section 7.9 for the [[Install]] algo is obviously incorrect and i think
overly complicated.
I don't think there's a need for that complexity. The primary purpose is to
force updates into use w/o having to reload (afaik). Here are 2 alternative
options that satisfy that primary purpose and avoid the complexity of mutating
other registrations and their clients.

1. SkipWating only affets pages that are controlled by an older version of the
same registration. So it's constrained to only apply to updates and not initial
installs.

2. SkipWaiting affects pages that are either controlled by an older version of
the same registration (as above) OR are not controlled by any serviceworker.
This variant allows 'skipwaiting' to jump into use upon initial installations
too.

Of these two options, I think #2 is the better choice.
 
In our impl, only those hosts already associated with the registration at the
time skipWaiting() is called would possibly be affected. This method doesn't
need to enumerate hosts. It can NotifyObervers of OnSkippedWaiting() having
happened.

Wdyt?

+    if (host->associated_registration() != this) {
+      host->DisassociateRegistration();
+      host->AssociateRegistration(this);
+    }
+  }
+}
+
 void ServiceWorkerRegistration::OnActivateEventFinished(
     ServiceWorkerVersion* activating_version,
     ServiceWorkerStatusCode status) {