ServiceWorker: add ServiceWorkerClients.claim() support (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 a4fac5c65873a9c9afe96c3b0e8b6fa92fe493b0..275747c31ded7956b37957a5169ff7d82fd16c8a 100644
--- a/content/browser/service_worker/service_worker_registration.cc
+++ b/content/browser/service_worker/service_worker_registration.cc
@@ -161,6 +161,31 @@ void ServiceWorkerRegistration::ActivateWaitingVersionWhenReady() {
     ActivateWaitingVersion();
 }
 
+void ServiceWorkerRegistration::ClaimClients() {
+  DCHECK(context_);
+  DCHECK(active_version());
+  // Live registrations contains longest matched registrations in database
+  // and new registrations on the fly. So we can just check these living ones.

[falken, 2015/01/29 15:10:37]

Maybe more clear to say: "It's sufficient to only search live registrations for
the longest match. The longest match is guaranteed to be live since it either
was the longest match at the time the provider host was created, or else was
created afterward."

[michaeln, 2015/01/29 22:35:34]

Not so sure about this comment? I think its incorrect. Suppose there's a better
match that did not claim upon coming into being. It got registered *after* the
document/host was loaded but did not claim and has since been dropped from the
live collection, as there's nothing preventing that. Am i missing something?

Perhaps if all new registrations for an origin (new being those that got
registered after the document/host was loaded) were guaranteed to be live, then
you could get away with only looking a the live collection. But i think nothing
nothing makes that guarantee atm.

ahha, if each host kept a ref to all new registrations made in its ori since it
loaded, i think that would do it.

+  std::vector<ServiceWorkerRegistrationInfo> infos =
+      context_->GetAllLiveRegistrationInfo();
+  for (scoped_ptr<ServiceWorkerContextCore::ProviderHostIterator> it =
+           context_->GetProviderHostIterator();
+       !it->IsAtEnd(); it->Advance()) {
+    ServiceWorkerProviderHost* host = it->GetProviderHost();
+    if (!ShouldClaim(host, infos))
+      continue;
+    if (host->associated_registration() == this) {
+      host->ClaimedByNewVersion();
+      continue;
+    }
+    host->DisassociateRegistration();

[falken, 2015/01/29 15:10:37]

It seems strange to preemptively Disassociate and then possibly not Associate
when CanAssociate is false.

[michaeln, 2015/01/29 22:35:34]

Agreed, the most direct thing to do might be to add an accessor for
host->allow_association() and test that directly at the top of the outer loop.

+    // There will still chance that a new registration triggers claim
+    // while a navigation is happening.

[falken, 2015/01/29 15:10:37]

I'm not sure what this comment is intended to be about. Is it explaining why
CanAssociateRegistration is being called? If so, maybe "CanAssociateRegistration
may be false while a navigation is happening."? But I'm not sure that's so
helpful.

+    if (host->CanAssociateRegistration(this))
+      host->AssociateRegistration(this);
+  }
+}
+
 void ServiceWorkerRegistration::ClearWhenReady() {
   DCHECK(context_);
   if (is_uninstalling_)
@@ -360,4 +385,31 @@ void ServiceWorkerRegistration::OnRestoreFinished(
   callback.Run(status);
 }
 
+bool ServiceWorkerRegistration::ShouldClaim(
+    ServiceWorkerProviderHost* provider_host,
+    std::vector<ServiceWorkerRegistrationInfo>& registration_infos) {
+  if (!ServiceWorkerUtils::ScopeMatches(
+        pattern_, provider_host->document_url()))
+    return false;
+
+  LongestScopeMatcher matcher(provider_host->document_url());
+  int64 match = kInvalidServiceWorkerRegistrationId;
+  for (std::vector<ServiceWorkerRegistrationInfo>::iterator it =
+           registration_infos.begin();
+       it != registration_infos.end(); ++it) {
+    ServiceWorkerRegistration* registration =
+        context_->GetLiveRegistration(it->registration_id);
+    if (!registration || registration->is_uninstalled())

[michaeln, 2015/01/29 22:35:34]

I think FindRegistrationForDocument would return 'this' over a registration that
does not yet have an active or waiting version, this logic will return a
registration that is still being downloaded/installed.

[xiang, 2015/01/30 07:18:57]

Yes, I think so, the spec defined this behavior.

+      continue;
+    if (matcher.MatchLongest(it->pattern))
+      match = it->registration_id;
+  }
+
+  if (match != registration_id_)
+    return false;
+  if (provider_host->controlling_version() == active_version())
+    return false;

[falken, 2015/01/29 15:10:37]

small optimization: we could move this check to the beginning of the function.

[michaeln, 2015/01/29 22:35:34]

another small opt: seed the matcher with the match for 'this', continue when you
see 'this' in the inner loop, return false early if the matcher matches
anything.

[xiang, 2015/01/30 07:18:57]

Done.

+  return true;
+}
+
 }  // namespace content