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

content/browser/service_worker/service_worker_registration.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/browser/service_worker/service_worker_registration.h"
 
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_info.h"
 #include "content/browser/service_worker/service_worker_register_job.h"
 #include "content/browser/service_worker/service_worker_utils.h"
@@ skipping 192 matching lines @@
     Clear();
   else if (should_activate_when_ready_)
     ActivateWaitingVersion();
   is_uninstalling_ = false;
   should_activate_when_ready_ = false;
 }
 
 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();
 
   if (activating_version->is_doomed() ||
       activating_version->status() == ServiceWorkerVersion::REDUNDANT) {
     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());
     // TODO(michaeln): 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));
     // "3. Run the [[UpdateState]] algorithm passing exitingWorker and
     // "redundant" as the arguments."
     exiting_version->SetStatus(ServiceWorkerVersion::REDUNDANT);
   }
 
   // "5. Set serviceWorkerRegistration.activeWorker to activatingWorker."
   // "6. Set serviceWorkerRegistration.waitingWorker to null."
   SetActiveVersion(activating_version.get());
 
   // "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) {
   if (!context_ || activating_version != active_version())
     return;
   // TODO(kinuko,falken): For some error cases (e.g. ServiceWorker is
   // unexpectedly terminated) we may want to retry sending the event again.
   if (status != SERVICE_WORKER_OK) {
     // "11. If activateFailed is true, then:..."
     UnsetVersion(activating_version);
@@ skipping 66 matching lines @@
   if (!context_) {
     callback.Run(SERVICE_WORKER_ERROR_ABORT);
     return;
   }
   context_->storage()->NotifyDoneInstallingRegistration(
       this, version.get(), status);
   callback.Run(status);
 }
 
 }  // namespace content