ServiceWorker: add ServiceWorkerClients.claim() support (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 143 matching lines @@
 }
 
 void ServiceWorkerRegistration::ActivateWaitingVersionWhenReady() {
   DCHECK(waiting_version());
   should_activate_when_ready_ = true;
   if (!active_version() || !active_version()->HasControllee() ||
       waiting_version()->skip_waiting())
     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_)
     return;
   is_uninstalling_ = true;
 
   context_->storage()->NotifyUninstallingRegistration(this);
   context_->storage()->DeleteRegistration(
       id(),
       pattern().GetOrigin(),
@@ skipping 179 matching lines @@
     ServiceWorkerStatusCode status) {
   if (!context_) {
     callback.Run(SERVICE_WORKER_ERROR_ABORT);
     return;
   }
   context_->storage()->NotifyDoneInstallingRegistration(
       this, version.get(), status);
   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