ServiceWorker: Support longest-prefix-match for registration scope

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 4ddd57bf2b52fd0c0567148ddf4571bdbb7e9988..fc196619cd9e0d14c28614bbd6ad5e9ae12d8a28 100644
--- a/content/browser/service_worker/service_worker_register_job.cc
+++ b/content/browser/service_worker/service_worker_register_job.cc
@@ -399,17 +399,26 @@ void ServiceWorkerRegisterJob::ResolvePromise(
 
 void ServiceWorkerRegisterJob::AssociatePendingVersionToDocuments(
     ServiceWorkerVersion* version) {

[michaeln, 2014/05/22 00:56:29]

when the input is NULL, this method should only affect those host that are
current associated with this registration. as coded it can alter hosts that have
an association to a different registration

-  // TODO(michaeln): This needs to respect the longest prefix wins
-  // when it comes to finding a registration for a document url.
-  // This should utilize storage->FindRegistrationForDocument().
   for (scoped_ptr<ServiceWorkerContextCore::ProviderHostIterator> it =
            context_->GetProviderHostIterator();
        !it->IsAtEnd();
        it->Advance()) {
-    ServiceWorkerProviderHost* provider_host = it->GetProviderHost();
-    if (ServiceWorkerUtils::ScopeMatches(pattern_,
-                                         provider_host->document_url()))
-      provider_host->SetPendingVersion(version);
+    ServiceWorkerProviderHost* host = it->GetProviderHost();
+    ServiceWorkerUtils::MatchResult result =
+        ServiceWorkerUtils::ScopeMatches(pattern_, host->document_url());
+    if (result == ServiceWorkerUtils::SCOPE_NO_MATCH)
+      continue;
+    if (result == ServiceWorkerUtils::SCOPE_EXACT_MATCH ||

[jsbell, 2014/05/21 19:20:03]

The spec doesn't call out an exact match as a special case.

This is reflected in the unit tests where "http://www.example.com/scope/foobar"
is considered a better match than "http://www.example.com/scope/foobar*"

This should either match the spec or we should file a spec bug and reference it
from a comment here.

[nhiroki, 2014/05/22 00:02:18]

Filed a spec bug: https://github.com/slightlyoff/ServiceWorker/issues/287

I'm thinking about dropping the exact-match case for now.

+        !host->pending_version()) {
+      host->SetPendingVersion(version);

[michaeln, 2014/05/22 00:56:29]

What if active_version is from an entirely different registration? I think we
can't mix and match version from different registrations on the same host. If
there is some assocation to a different registration on the document (pending or
active)... i think this method needs to leave it alone.

[dominicc (has gone to gerrit), 2014/05/22 05:03:31]

FWIW there are some related comments on the spec bug about .active, .waiting
properties <https://github.com/slightlyoff/ServiceWorker/issues/232>

Maybe we as implementers should decide what makes sense here, make a specific
proposal, and just optimistically implement that.

I know we don't mix versions from different registrations, but why? What breaks
if that happens? I am wondering if we're missing an object and that is what
makes this hard to model.

[michaeln, 2014/05/23 01:23:27]

I think its the intent of the spec to not mix-n-match. Is there a compelling
reason to mix registrations like that? Seems like it would add complexity for
webdevs and brwsrdevs alike, and for something that isn't really wanted or
needed.
If a page is associated with an intalled/active sw, that registration may itself
be updating and something shows up in the 'downloading' or 'installing' state.
But then if a new registration with a longer scope comes along, that too may
result in a new version appearing in the 'downloading' or 'installing' state.

+      continue;
+    }
+
+    DCHECK_EQ(ServiceWorkerUtils::SCOPE_WILDCARD_MATCH, result);
+    if (ServiceWorkerUtils::CompareScopePriorities(
+            pattern_, host->pending_version()->scope()) >= 0) {

[jsbell, 2014/05/21 19:20:03]

If result here == 0 that would mean the scopes were the same - is there anything
we can assert in that case?

[michaeln, 2014/05/22 00:56:29]

per above, once there is some association on place, i think we leave it alone
for the life of that doc

also, SetPendingVersion results in ipc traffic and events and such. We should
pick one, then call that method once for a given host. As implemented, this loop
can spam that method.

+      host->SetPendingVersion(version);
+    }
   }
 }