ServiceWorker: add ServiceWorkerClients.claim() support (2/3).

ServiceWorker: add ServiceWorkerClients.claim() support (2/3).

The claiming registration will fetch all registrations from
DB and comparing them with existing provider hosts. If it
has the longest match, it will start controlling the
provider host.

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-claim

(1/3): https://codereview.chromium.org/868463004/
(2/3): This
(3/3): https://codereview.chromium.org/872593002/

BUG=450106
TEST=https://codereview.chromium.org/872593002/

Committed: https://crrev.com/afc4f16f4cf1b894cef3c35b806d32ef644ae3b5
Cr-Commit-Position: refs/heads/master@{#314514}

[xiang, 2015-01-23 04:41:14]

Patchset #1 (id:1) has been deleted

[xiang, 2015-01-23 04:59:09]

xiang.long@intel.com changed reviewers:
+ falken@chromium.org, michaeln@chromium.org

[xiang, 2015-01-23 04:59:38]

PTAL, thanks.

[falken, 2015-01-26 08:45:13]

this is looking pretty good

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:92:
SetControllerVersionAttribute(active_version, true);
nit: can you add /* should_notify_controller_change */ next to true, or use a
const boolean.

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:254: if
(associated_registration_ == registration) {
when does this happen? if this provider host is associated with the
registration, wouldn't we get notification when the active version changes?

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:259:
AssociateRegistration(registration, true);
/* should_notify_controller_change */

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:145: void
SetController(ServiceWorkerRegistration* registration);
This API gets a bit confusing: this function is a lot like
AssociateRegistration. Is it possible to fold them into one have better naming
to make clear which one does what?

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:76: "Only active worker
can claim clients.";
s/Only active/Only the active/

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:79: "Failed to claim
clients due to ServiceWorker system shutdown.";
s/ServiceWorker/Service Worker/

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1132: }
maybe add DCHECK_EQ(this, registration->active_version()) here

https://codereview.chromium.org/871853002/diff/40001/content/common/service_w...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/871853002/diff/40001/content/common/service_w...
content/common/service_worker/service_worker_messages.h:244: // Asks the browser
to take over pages should belong to this worker.
nit: comment is vague, the browser itself doesn't take over pages. Maybe "Asks
the browser to have this worker take control of pages that match its scope."

[falken, 2015-01-26 08:45:41]

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:145: void
SetController(ServiceWorkerRegistration* registration);
On 2015/01/26 08:45:13, falken wrote:
> This API gets a bit confusing: this function is a lot like
> AssociateRegistration. Is it possible to fold them into one have better naming
> to make clear which one does what?

I meant to write: fold them into one OR have better naming

[michaeln, 2015-01-28 00:55:14]

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:99: bool
should_notify_controller_change = false);
This flag seems odd? Also it shouldn't have a default value per the style guide.

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:145: void
SetController(ServiceWorkerRegistration* registration);
> I meant to write: fold them into one OR have better naming

Agreed, please don't use SetController() as the name here.

OnClaimedByNewVersion(reg) would be in keeping with how the similar
OnSkippedWaiting(reg) is handled. Maybe this could be broken into two parts.

- first rejigger the associated registrations
- then notify them of the being claimed by a new version

That would be even more in keeping with how the similar skipped waiting is
handled. Might help get rid of that odd flag too?

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:178: for
(std::vector<ServiceWorkerRegistrationInfo>::iterator it =
This inner loop is not so nice. I think we can avoid looping over all
registrations. We only care about hosts for which we're a match, so that test
can go first, and then we only need to know if we're better than its current
match. Its easier to explain like this...

// first rejigger associations
foreach host
  if shouldclaim(host)
    if (host->reg != this) {
      host->DisassociateReg()
      host->AssociateReg(this);
    }

// notify associated hosts about being claimed by a new version
FOR_EACH_OBSERVER(Listener, listeners_,
                  OnClaimedByNewVersion(this));
return;

bool shouldclaim(host) {
   if (host->asssociated_reg == this)
     return true;  // it's one of ours and the new version should take over
   LongestScopeMatcher matcher(host->document_url());
   if (!matcher.Match(this->pattern))
     return false; // no point looking any further
   // see if we're a better match than its current association
   if (!host->associated_reg)
     return true;  // we're definitely better than nothing
   if (matcher.Match(host->associated_reg()->pattern))
     return false; // its current association wins
   return true; // hooray, we win
}

[xiang, 2015-01-28 05:31:14]

Patchset #3 (id:60001) has been deleted

[xiang, 2015-01-28 05:38:35]

Thanks for the review! CL updated, PTAL.

I have removed the should_notify_controllerchange flag, a UseRegistration()
method is introduced which I think is better to describe the behavior.

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:92:
SetControllerVersionAttribute(active_version, true);
On 2015/01/26 08:45:13, falken wrote:
> nit: can you add /* should_notify_controller_change */ next to true, or use a
> const boolean.

Done.

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:254: if
(associated_registration_ == registration) {
On 2015/01/26 08:45:12, falken wrote:
> when does this happen? if this provider host is associated with the
> registration, wouldn't we get notification when the active version changes?

This could happen when a registration associated this provider host when
"registering" itself and later the claims clients.

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:259:
AssociateRegistration(registration, true);
On 2015/01/26 08:45:12, falken wrote:
> /* should_notify_controller_change */

Done. We don't need the flags now.

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:99: bool
should_notify_controller_change = false);
On 2015/01/28 00:55:13, michaeln wrote:
> This flag seems odd? Also it shouldn't have a default value per the style
guide.
> 

Done.

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:178: for
(std::vector<ServiceWorkerRegistrationInfo>::iterator it =
On 2015/01/28 00:55:13, michaeln wrote:
> This inner loop is not so nice. I think we can avoid looping over all
> registrations. We only care about hosts for which we're a match, so that test
> can go first, and then we only need to know if we're better than its current
> match. Its easier to explain like this...
> 
> // first rejigger associations
> foreach host
>   if shouldclaim(host)
>     if (host->reg != this) {
>       host->DisassociateReg()
>       host->AssociateReg(this);
>     }
> 
> // notify associated hosts about being claimed by a new version
> FOR_EACH_OBSERVER(Listener, listeners_,
>                   OnClaimedByNewVersion(this));
> return;
> 
> bool shouldclaim(host) {
>    if (host->asssociated_reg == this)
>      return true;  // it's one of ours and the new version should take over
>    LongestScopeMatcher matcher(host->document_url());
>    if (!matcher.Match(this->pattern))
>      return false; // no point looking any further
>    // see if we're a better match than its current association
>    if (!host->associated_reg)
>      return true;  // we're definitely better than nothing
>    if (matcher.Match(host->associated_reg()->pattern))
>      return false; // its current association wins
>    return true; // hooray, we win
> }

Yes, move out the inner loop is more cleaner.

However the from the spec: "Let registration be the result of running Match
Service Worker Registration algorithm passing client's creation url as the
argument". The "better match than its current association" way will make some
cases different.

For example: foobar.html is using "/foo" registration, now "/foob" & "/foobar"
registrations both registered and activated, then "/foob" triggered .claim().
From spec, the foobar.html should stilling using "/foo" instead of start using
"/foob". I feel this should be the right behavior even it maybe not a practical
scenario, wdyt?

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:76: "Only active worker
can claim clients.";
On 2015/01/26 08:45:13, falken wrote:
> s/Only active/Only the active/

Done.

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:79: "Failed to claim
clients due to ServiceWorker system shutdown.";
On 2015/01/26 08:45:13, falken wrote:
> s/ServiceWorker/Service Worker/

Done.

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1132: }
On 2015/01/26 08:45:13, falken wrote:
> maybe add DCHECK_EQ(this, registration->active_version()) here

Done.

https://codereview.chromium.org/871853002/diff/40001/content/common/service_w...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/871853002/diff/40001/content/common/service_w...
content/common/service_worker/service_worker_messages.h:244: // Asks the browser
to take over pages should belong to this worker.
On 2015/01/26 08:45:13, falken wrote:
> nit: comment is vague, the browser itself doesn't take over pages. Maybe "Asks
> the browser to have this worker take control of pages that match its scope."

Done.

[falken, 2015-01-28 08:59:50]

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:254: if
(associated_registration_ == registration) {
On 2015/01/28 05:38:35, xiang wrote:
> On 2015/01/26 08:45:12, falken wrote:
> > when does this happen? if this provider host is associated with the
> > registration, wouldn't we get notification when the active version changes?
> 
> This could happen when a registration associated this provider host when
> "registering" itself and later the claims clients.

I see, I was confused and thought SWProviderHost listens for attribute changes
in SWRegistration. It used to do this, but no longer does. The comment for
AssociateRegistration() should be changed.

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:178: for
(std::vector<ServiceWorkerRegistrationInfo>::iterator it =
On 2015/01/28 05:38:35, xiang wrote:
> On 2015/01/28 00:55:13, michaeln wrote:
> > This inner loop is not so nice. I think we can avoid looping over all
> > registrations. We only care about hosts for which we're a match, so that
test
> > can go first, and then we only need to know if we're better than its current
> > match. Its easier to explain like this...
> > 
> > // first rejigger associations
> > foreach host
> >   if shouldclaim(host)
> >     if (host->reg != this) {
> >       host->DisassociateReg()
> >       host->AssociateReg(this);
> >     }
> > 
> > // notify associated hosts about being claimed by a new version
> > FOR_EACH_OBSERVER(Listener, listeners_,
> >                   OnClaimedByNewVersion(this));
> > return;
> > 
> > bool shouldclaim(host) {
> >    if (host->asssociated_reg == this)
> >      return true;  // it's one of ours and the new version should take over
> >    LongestScopeMatcher matcher(host->document_url());
> >    if (!matcher.Match(this->pattern))
> >      return false; // no point looking any further
> >    // see if we're a better match than its current association
> >    if (!host->associated_reg)
> >      return true;  // we're definitely better than nothing
> >    if (matcher.Match(host->associated_reg()->pattern))
> >      return false; // its current association wins
> >    return true; // hooray, we win
> > }
> 
> Yes, move out the inner loop is more cleaner.
> 
> However the from the spec: "Let registration be the result of running Match
> Service Worker Registration algorithm passing client's creation url as the
> argument". The "better match than its current association" way will make some
> cases different.
> 
> For example: foobar.html is using "/foo" registration, now "/foob" & "/foobar"
> registrations both registered and activated, then "/foob" triggered .claim().
> From spec, the foobar.html should stilling using "/foo" instead of start using
> "/foob". I feel this should be the right behavior even it maybe not a
practical
> scenario, wdyt?

I think you're right, "/foob" shouldn't claim the page. For correctness, I'd opt
to keep the original algorithm, although it may be slow.

Actually I'm wondering now, could there be registrations in storage but not
existing as "live" registrations in memory that could possibly be the longest
match?

https://codereview.chromium.org/871853002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/871853002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:99: void
UseRegistration(ServiceWorkerRegistration* registration);
This is better, but I'm still not a fan of this name. The user of this class
will likely be unsure when to use UseRegistration or AssociateRegistration.

I like Michael's suggestion:
- "first rejigger the associated registrations" (AssociateRegistration)
- "then notify them of the being claimed by a new version"
(OnClaimedByNewVersion)

https://codereview.chromium.org/871853002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:101: // Associates
to |registration| to listen for its version change events.
It's not part of your patch, but this class no longer listens for version change
events.

[michaeln, 2015-01-28 21:11:14]

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:178: for
(std::vector<ServiceWorkerRegistrationInfo>::iterator it =
On 2015/01/28 08:59:49, falken wrote:
> On 2015/01/28 05:38:35, xiang wrote:
> > On 2015/01/28 00:55:13, michaeln wrote:
> > > This inner loop is not so nice. I think we can avoid looping over all
> > > registrations. We only care about hosts for which we're a match, so that
> test
> > > can go first, and then we only need to know if we're better than its
current
> > > match. Its easier to explain like this...
> > > 
> > > // first rejigger associations
> > > foreach host
> > >   if shouldclaim(host)
> > >     if (host->reg != this) {
> > >       host->DisassociateReg()
> > >       host->AssociateReg(this);
> > >     }
> > > 
> > > // notify associated hosts about being claimed by a new version
> > > FOR_EACH_OBSERVER(Listener, listeners_,
> > >                   OnClaimedByNewVersion(this));
> > > return;
> > > 
> > > bool shouldclaim(host) {
> > >    if (host->asssociated_reg == this)
> > >      return true;  // it's one of ours and the new version should take
over
> > >    LongestScopeMatcher matcher(host->document_url());
> > >    if (!matcher.Match(this->pattern))
> > >      return false; // no point looking any further
> > >    // see if we're a better match than its current association
> > >    if (!host->associated_reg)
> > >      return true;  // we're definitely better than nothing
> > >    if (matcher.Match(host->associated_reg()->pattern))
> > >      return false; // its current association wins
> > >    return true; // hooray, we win
> > > }
> > 
> > Yes, move out the inner loop is more cleaner.
> > 
> > However the from the spec: "Let registration be the result of running Match
> > Service Worker Registration algorithm passing client's creation url as the
> > argument". The "better match than its current association" way will make
some
> > cases different.
> > 
> > For example: foobar.html is using "/foo" registration, now "/foob" &
"/foobar"
> > registrations both registered and activated, then "/foob" triggered
.claim().
> > From spec, the foobar.html should stilling using "/foo" instead of start
using
> > "/foob". I feel this should be the right behavior even it maybe not a
> practical
> > scenario, wdyt?

Thats too bad because it adds code complexity and will take longer to compute.

> 
> Actually I'm wondering now, could there be registrations in storage but not
> existing as "live" registrations in memory that could possibly be the longest
> match?

Yes, i think you'll have to use FindRegistrationForDocument(), that's an async
process so its definitely more complicated.

Also notice the SetAllowAssociation() method and how it's used by
ServiceWorkerControlleeRequestHandler to coordinate with
ServiceWorkerRegisterJob about who has the rights to determine the association
(the request handler always trumps). Now that there's a third agency that make
this decision (claim method handler), we have to figure out the priority of this
new agency?

Idk, we might want to suggest the spec change to make Claim a purely synch
decision making process (by just comparing the current associations to the new
claimee)? That would make it easier for vendors to impl and to actually match
one another in behavior.

If we could ensure that FindRegForDocument() completely syncly for a particular
origin when any association was active for that ori, that might help.

https://codereview.chromium.org/871853002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/871853002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_controllee_request_handler.cc:243:
provider_host_->UseRegistration(registration.get());
please stick with the verb associate, we have other names that resonate with it
and use this terminology in comments

[xiang, 2015-01-29 02:58:38]

Some comments before I update the CL.

I'm not sure on the registration match impl(better than current v.s. better than
all), should I file an issue on the spec and wait the decision?

Maybe just keep a living registrations per origin would make the "match all"
method faster and acceptable, wdyt?

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:178: for
(std::vector<ServiceWorkerRegistrationInfo>::iterator it =
On 2015/01/28 21:11:14, michaeln wrote:
> On 2015/01/28 08:59:49, falken wrote:
> > On 2015/01/28 05:38:35, xiang wrote:
> > > On 2015/01/28 00:55:13, michaeln wrote:
> > > > This inner loop is not so nice. I think we can avoid looping over all
> > > > registrations. We only care about hosts for which we're a match, so that
> > test
> > > > can go first, and then we only need to know if we're better than its
> current
> > > > match. Its easier to explain like this...
> > > > 
> > > > // first rejigger associations
> > > > foreach host
> > > >   if shouldclaim(host)
> > > >     if (host->reg != this) {
> > > >       host->DisassociateReg()
> > > >       host->AssociateReg(this);
> > > >     }
> > > > 
> > > > // notify associated hosts about being claimed by a new version
> > > > FOR_EACH_OBSERVER(Listener, listeners_,
> > > >                   OnClaimedByNewVersion(this));
> > > > return;
> > > > 
> > > > bool shouldclaim(host) {
> > > >    if (host->asssociated_reg == this)
> > > >      return true;  // it's one of ours and the new version should take
> over
> > > >    LongestScopeMatcher matcher(host->document_url());
> > > >    if (!matcher.Match(this->pattern))
> > > >      return false; // no point looking any further
> > > >    // see if we're a better match than its current association
> > > >    if (!host->associated_reg)
> > > >      return true;  // we're definitely better than nothing
> > > >    if (matcher.Match(host->associated_reg()->pattern))
> > > >      return false; // its current association wins
> > > >    return true; // hooray, we win
> > > > }
> > > 
> > > Yes, move out the inner loop is more cleaner.
> > > 
> > > However the from the spec: "Let registration be the result of running
Match
> > > Service Worker Registration algorithm passing client's creation url as the
> > > argument". The "better match than its current association" way will make
> some
> > > cases different.
> > > 
> > > For example: foobar.html is using "/foo" registration, now "/foob" &
> "/foobar"
> > > registrations both registered and activated, then "/foob" triggered
> .claim().
> > > From spec, the foobar.html should stilling using "/foo" instead of start
> using
> > > "/foob". I feel this should be the right behavior even it maybe not a
> > practical
> > > scenario, wdyt?
> 
> Thats too bad because it adds code complexity and will take longer to compute.
> 
> > 
> > Actually I'm wondering now, could there be registrations in storage but not
> > existing as "live" registrations in memory that could possibly be the
longest
> > match?
> 
> Yes, i think you'll have to use FindRegistrationForDocument(), that's an async
> process so its definitely more complicated.
> 
> Also notice the SetAllowAssociation() method and how it's used by
> ServiceWorkerControlleeRequestHandler to coordinate with
> ServiceWorkerRegisterJob about who has the rights to determine the association
> (the request handler always trumps). Now that there's a third agency that make
> this decision (claim method handler), we have to figure out the priority of
this
> new agency?
> 
> Idk, we might want to suggest the spec change to make Claim a purely synch
> decision making process (by just comparing the current associations to the new
> claimee)? That would make it easier for vendors to impl and to actually match
> one another in behavior.
> 
> If we could ensure that FindRegForDocument() completely syncly for a
particular
> origin when any association was active for that ori, that might help.

I thought about FindRegistrationForDocument() when wrote these code, maybe a
comment should be added to describe the trick. AFAIK there should no better
match for a document in storage than in living registrations. The living
registrations contain both stored registrations found in document navigation and
newly registered ones. So we don't need to go back to DB, is this correct?

Thanks for mention the SetAllowAssociation(), I should check
CanAssociateRegistration() before association when claim happens.

I think claim() will not influence current ServiceWorkerControlleeRequestHandler
decision on whether an association should be happened or not, as claim() only
effects when worker is "active" which means the corresponding registration data
must already update to DB. If FindForDocumentInDB() send before a claim
registration WriteRegistrationInDB() the later claim() will work; if
FindForDocumentInDB() send after a claim registration WriteRegistrationInDB()
the registration will be picked.

https://codereview.chromium.org/871853002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/871853002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:99: void
UseRegistration(ServiceWorkerRegistration* registration);
On 2015/01/28 08:59:49, falken wrote:
> This is better, but I'm still not a fan of this name. The user of this class
> will likely be unsure when to use UseRegistration or AssociateRegistration.
> 
> I like Michael's suggestion:
> - "first rejigger the associated registrations" (AssociateRegistration)
> - "then notify them of the being claimed by a new version"
> (OnClaimedByNewVersion)

You are right, the "use" will confuse people without the spec context. I will
change it.
The OnClaimedByNewVersion() seems not needed, if we make a "claiming" flag in
version.

[falken, 2015-01-29 03:28:51]

> I'm not sure on the registration match impl(better than current v.s. better
than
> all), should I file an issue on the spec and wait the decision?

It's a rather obscure case, we don't need to block on it. We can always land
something for now and file a bug to track the spec issue if we raise one.

But I'm feeling the latest patchset is fine. The inner loop doesn't seem too
costly since we only enter it for provider hosts in the claiming reg's scope.

> Maybe just keep a living registrations per origin would make the "match all"
> method faster and acceptable, wdyt?

This is a potentially worthwhile optimization, though I'd rather do it in a
separate patch.

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/871853002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:178: for
(std::vector<ServiceWorkerRegistrationInfo>::iterator it =
On 2015/01/29 02:58:37, xiang wrote:
> On 2015/01/28 21:11:14, michaeln wrote:
> > On 2015/01/28 08:59:49, falken wrote:
> > > On 2015/01/28 05:38:35, xiang wrote:
> > > > On 2015/01/28 00:55:13, michaeln wrote:
> > > > > This inner loop is not so nice. I think we can avoid looping over all
> > > > > registrations. We only care about hosts for which we're a match, so
that
> > > test
> > > > > can go first, and then we only need to know if we're better than its
> > current
> > > > > match. Its easier to explain like this...
> > > > > 
> > > > > // first rejigger associations
> > > > > foreach host
> > > > >   if shouldclaim(host)
> > > > >     if (host->reg != this) {
> > > > >       host->DisassociateReg()
> > > > >       host->AssociateReg(this);
> > > > >     }
> > > > > 
> > > > > // notify associated hosts about being claimed by a new version
> > > > > FOR_EACH_OBSERVER(Listener, listeners_,
> > > > >                   OnClaimedByNewVersion(this));
> > > > > return;
> > > > > 
> > > > > bool shouldclaim(host) {
> > > > >    if (host->asssociated_reg == this)
> > > > >      return true;  // it's one of ours and the new version should take
> > over
> > > > >    LongestScopeMatcher matcher(host->document_url());
> > > > >    if (!matcher.Match(this->pattern))
> > > > >      return false; // no point looking any further
> > > > >    // see if we're a better match than its current association
> > > > >    if (!host->associated_reg)
> > > > >      return true;  // we're definitely better than nothing
> > > > >    if (matcher.Match(host->associated_reg()->pattern))
> > > > >      return false; // its current association wins
> > > > >    return true; // hooray, we win
> > > > > }
> > > > 
> > > > Yes, move out the inner loop is more cleaner.
> > > > 
> > > > However the from the spec: "Let registration be the result of running
> Match
> > > > Service Worker Registration algorithm passing client's creation url as
the
> > > > argument". The "better match than its current association" way will make
> > some
> > > > cases different.
> > > > 
> > > > For example: foobar.html is using "/foo" registration, now "/foob" &
> > "/foobar"
> > > > registrations both registered and activated, then "/foob" triggered
> > .claim().
> > > > From spec, the foobar.html should stilling using "/foo" instead of start
> > using
> > > > "/foob". I feel this should be the right behavior even it maybe not a
> > > practical
> > > > scenario, wdyt?
> > 
> > Thats too bad because it adds code complexity and will take longer to
compute.
> > 
> > > 
> > > Actually I'm wondering now, could there be registrations in storage but
not
> > > existing as "live" registrations in memory that could possibly be the
> longest
> > > match?
> > 
> > Yes, i think you'll have to use FindRegistrationForDocument(), that's an
async
> > process so its definitely more complicated.
> > 
> > Also notice the SetAllowAssociation() method and how it's used by
> > ServiceWorkerControlleeRequestHandler to coordinate with
> > ServiceWorkerRegisterJob about who has the rights to determine the
association
> > (the request handler always trumps). Now that there's a third agency that
make
> > this decision (claim method handler), we have to figure out the priority of
> this
> > new agency?
> > 
> > Idk, we might want to suggest the spec change to make Claim a purely synch
> > decision making process (by just comparing the current associations to the
new
> > claimee)? That would make it easier for vendors to impl and to actually
match
> > one another in behavior.
> > 
> > If we could ensure that FindRegForDocument() completely syncly for a
> particular
> > origin when any association was active for that ori, that might help.
> 
> I thought about FindRegistrationForDocument() when wrote these code, maybe a
> comment should be added to describe the trick. AFAIK there should no better
> match for a document in storage than in living registrations. The living
> registrations contain both stored registrations found in document navigation
and
> newly registered ones. So we don't need to go back to DB, is this correct?

I suspected this might be the case too. Michael, what do you think? Definitely a
comment should be added about the "trick".

> 
> Thanks for mention the SetAllowAssociation(), I should check
> CanAssociateRegistration() before association when claim happens.
> 
> I think claim() will not influence current
ServiceWorkerControlleeRequestHandler
> decision on whether an association should be happened or not, as claim() only
> effects when worker is "active" which means the corresponding registration
data
> must already update to DB. If FindForDocumentInDB() send before a claim
> registration WriteRegistrationInDB() the later claim() will work; if
> FindForDocumentInDB() send after a claim registration WriteRegistrationInDB()
> the registration will be picked.

[xiang, 2015-01-29 06:23:48]

On 2015/01/29 03:28:51, falken wrote:
> > I'm not sure on the registration match impl(better than current v.s. better
> than
> > all), should I file an issue on the spec and wait the decision?
> 
> It's a rather obscure case, we don't need to block on it. We can always land
> something for now and file a bug to track the spec issue if we raise one.
> 
> But I'm feeling the latest patchset is fine. The inner loop doesn't seem too
> costly since we only enter it for provider hosts in the claiming reg's scope.
> 
> > Maybe just keep a living registrations per origin would make the "match all"
> > method faster and acceptable, wdyt?
> 
> This is a potentially worthwhile optimization, though I'd rather do it in a
> separate patch.
> 

Thank you for the clarification. CL updated.

[xiang, 2015-01-29 09:06:40]

Update the CL again, as previous one has an obvious error.
I think we still don't need the OnClaimedByNewVersion() method.

[falken, 2015-01-29 15:10:38]

This basically looks good to me.

I think there's some better refactoring of (Can)(Dis)AssociateRegistration and
ClaimedByNewVersion possible but I don't have a better idea now.

I'm confused by "Currently we will reject claim request when worker is not
active." in the description. Isn't that the correct behavior per spec?

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:147: // Set
controller to the active version of associated registration.
nit: "Sets controller to the..."

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:168: // and new
registrations on the fly. So we can just check these living ones.
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."

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:181:
host->DisassociateRegistration();
It seems strange to preemptively Disassociate and then possibly not Associate
when CanAssociate is false.

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:183: // while a
navigation is happening.
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.

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:411: return false;
small optimization: we could move this check to the beginning of the function.

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1126:
ServiceWorkerRegistration* registration = NULL;
nullptr

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_version.h:292: bool
claiming_clients() const { return claiming_clients_; }
I'd prefer is_claiming_clients(), otherwise it looks like it'd return an array
of clients.

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

This looks pretty good to me too.

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:175: void
SetControllerVersionAttribute(ServiceWorkerVersion* version);
I think you can get rid of the ver->is_claiming_ flag with a new arg to this
private method. When its called thru ClaimedBy its true otherwise false.

My push back against the bool input arg in AssociateRegistration in earlier
snapshots was mostly because it was a public method. Since this class is
responsible for sending those IPCs only this class calls this method... fine and
good to add the param here.

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:168: // and new
registrations on the fly. So we can just check these living ones.
On 2015/01/29 15:10:37, falken wrote:
> 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."

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.

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:181:
host->DisassociateRegistration();
On 2015/01/29 15:10:37, falken wrote:
> It seems strange to preemptively Disassociate and then possibly not Associate
> when CanAssociate is false.

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.

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:402: if
(!registration || registration->is_uninstalled())
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.

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:411: return false;
On 2015/01/29 15:10:37, falken wrote:
> small optimization: we could move this check to the beginning of the function.

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.

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_version.h:292: bool
claiming_clients() const { return claiming_clients_; }
I think this isn't needed, see comments in the provider host class.

[xiang, 2015-01-30 07:01:11]

Patchset #6 (id:130001) has been deleted

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

Thanks for the review. PTAL.

As michaeln@ commented, depends on live registrations is not safe when a
ServiceWorkerRegistration object gone. So before we really think this is a
performance issue, I changed the "claim" process from sync to async. Now it will
check registrations fetched from DB and installing ones. Sorry the previous
immature optimization.

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:402: if
(!registration || registration->is_uninstalled())
On 2015/01/29 22:35:34, michaeln wrote:
> 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.

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

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:411: return false;
On 2015/01/29 22:35:34, michaeln wrote:
> On 2015/01/29 15:10:37, falken wrote:
> > small optimization: we could move this check to the beginning of the
function.
> 
> 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.

Done.

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/871853002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1126:
ServiceWorkerRegistration* registration = NULL;
On 2015/01/29 15:10:37, falken wrote:
> nullptr

Done.

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:99: is_claiming_
= true;
A flag is needed here because AssociateRegistration() will also call
SetControllerVersionAttribute() and trigger controllerchange event.

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:409: if
(registration && registration->is_uninstalled())
Only live registrations maybe uninstalled during GetRegistrationsForOrigin().

[falken, 2015-01-30 10:24:02]

It's unfortunate that claim() hits DB before doing anything but we can land and
iterate. I figure the use case for claim() is for a new registration to control
pages on initial navigation and we're bounded by DB anyway because it has to be
stored first.

We have some ideas for removing the DB read and can also file a spec bug if it
turns out too pernicious.

So lgtm unless Michael has objections.

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:103:
DisassociateRegistration();
As mentioned before, it looks wrong to Disassociate and later possibly not
Associate if CanAssociate is false. Could you make this else  an else if
(allow_registration_)?

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:168: // enough for
claiming documents.
I'd change this comment to a TODO to not have to hit DB and link to a bug.

[michaeln, 2015-01-30 23:43:07]

>> 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.
>
> Yes, I think so, the spec defined this behavior.

And so it does.

Although, that seems like it's probably an oversight. It's odd that Claim() uses
ever so slightly different logic to pick a registration than that used by
HandleFetch() for a navigation?

I think the verbage for Claim is inadvertently missing something like:
"3. If registration's active worker is null continue to the next iteration"

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:103:
DisassociateRegistration();
On 2015/01/30 10:24:02, falken wrote:
> As mentioned before, it looks wrong to Disassociate and later possibly not
> Associate if CanAssociate is false. Could you make this else  an else if
> (allow_registration_)?

+1 if (allow_registration_) prior to calling DisassociateRegistration

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:411: if
(matcher.MatchLongest(info.pattern))
given a docurl and a matchingscope, this function is relying on the matcher
return false for a 2nd call with the same matching inpu.

LongestScopeMatcher matcher(docurl);
ASSERT_TRUE(matcher->MatchLongest(matchingscope));
ASSERT_FALSE(matcher->MatchLongest(matchingscope));

Can you add a test like that to service_worker_utils_unittest.cc

[michaeln, 2015-01-30 23:43:51]

On 2015/01/30 23:43:07, michaeln wrote:
> >> 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.
> >
> > Yes, I think so, the spec defined this behavior.
> 
> And so it does.
> 
> Although, that seems like it's probably an oversight. It's odd that Claim()
uses
> ever so slightly different logic to pick a registration than that used by
> HandleFetch() for a navigation?
> 
> I think the verbage for Claim is inadvertently missing something like:
> "3. If registration's active worker is null continue to the next iteration"
> 
>
https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
> File content/browser/service_worker/service_worker_provider_host.cc (right):
> 
>
https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
> content/browser/service_worker/service_worker_provider_host.cc:103:
> DisassociateRegistration();
> On 2015/01/30 10:24:02, falken wrote:
> > As mentioned before, it looks wrong to Disassociate and later possibly not
> > Associate if CanAssociate is false. Could you make this else  an else if
> > (allow_registration_)?
> 
> +1 if (allow_registration_) prior to calling DisassociateRegistration
> 
>
https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
> File content/browser/service_worker/service_worker_registration.cc (right):
> 
>
https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
> content/browser/service_worker/service_worker_registration.cc:411: if
> (matcher.MatchLongest(info.pattern))
> given a docurl and a matchingscope, this function is relying on the matcher
> return false for a 2nd call with the same matching inpu.
> 
> LongestScopeMatcher matcher(docurl);
> ASSERT_TRUE(matcher->MatchLongest(matchingscope));
> ASSERT_FALSE(matcher->MatchLongest(matchingscope));
> 
> Can you add a test like that to service_worker_utils_unittest.cc

lgtm

[xiang, 2015-02-02 04:24:59]

Thank you for the review!

I filed a crbug for DB access at:
http://code.google.com/p/chromium/issues/detail?id=454250

I also raised Michael's concern at:
https://github.com/slightlyoff/ServiceWorker/issues/586#issuecomment-72404374

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:103:
DisassociateRegistration();
On 2015/01/30 23:43:07, michaeln wrote:
> On 2015/01/30 10:24:02, falken wrote:
> > As mentioned before, it looks wrong to Disassociate and later possibly not
> > Associate if CanAssociate is false. Could you make this else  an else if
> > (allow_registration_)?
> 
> +1 if (allow_registration_) prior to calling DisassociateRegistration

Done.

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:168: // enough for
claiming documents.
On 2015/01/30 10:24:02, falken wrote:
> I'd change this comment to a TODO to not have to hit DB and link to a bug.

Done.

https://codereview.chromium.org/871853002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:411: if
(matcher.MatchLongest(info.pattern))
On 2015/01/30 23:43:07, michaeln wrote:
> given a docurl and a matchingscope, this function is relying on the matcher
> return false for a 2nd call with the same matching inpu.
> 
> LongestScopeMatcher matcher(docurl);
> ASSERT_TRUE(matcher->MatchLongest(matchingscope));
> ASSERT_FALSE(matcher->MatchLongest(matchingscope));
> 
> Can you add a test like that to service_worker_utils_unittest.cc

Done.

[xiang, 2015-02-02 04:39:43]

xiang.long@intel.com changed reviewers:
+ johnme@chromium.org, mkwst@chromium.org

[xiang, 2015-02-02 04:40:10]

johnme@ would you please help to review: content/browser/push_messaging/
changes?
mkwst@ would you please help to review:  content/browser/notifications/ &
content/common/service_worker/service_worker_messages.h changes?
Thanks!

[Mike West, 2015-02-02 08:33:53]

IPC LGTM.

[xiang, 2015-02-03 06:33:07]

On 2015/02/02 04:40:10, xiang wrote:
> johnme@ would you please help to review: content/browser/push_messaging/
> changes?
> mkwst@ would you please help to review:  content/browser/notifications/ &
> content/common/service_worker/service_worker_messages.h changes?
> Thanks!

ping johnme@, do you have time to take a look? thanks.

[mlamouri (slow - plz ping), 2015-02-03 09:41:00]

On 2015/02/03 at 06:33:07, xiang.long wrote:
> On 2015/02/02 04:40:10, xiang wrote:
> > johnme@ would you please help to review: content/browser/push_messaging/
> > changes?
> > mkwst@ would you please help to review:  content/browser/notifications/ &
> > content/common/service_worker/service_worker_messages.h changes?
> > Thanks!
> 
> ping johnme@, do you have time to take a look? thanks.

not OWNER but lgtm.

[johnme, 2015-02-03 09:59:20]

rubber stamp lgtm

[xiang, 2015-02-04 05:11:39]

The CQ bit was checked by xiang.long@intel.com

[commit-bot: I haz the power, 2015-02-04 05:12:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/871853002/170001

[commit-bot: I haz the power, 2015-02-04 05:44:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-04 05:44:25]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...)

[xiang, 2015-02-04 06:09:14]

The CQ bit was checked by xiang.long@intel.com

[commit-bot: I haz the power, 2015-02-04 06:09:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/871853002/190001

[commit-bot: I haz the power, 2015-02-04 07:05:27]

Committed patchset #8 (id:190001)

[commit-bot: I haz the power, 2015-02-04 07:06:21]

Patchset 8 (id:??) landed as
https://crrev.com/afc4f16f4cf1b894cef3c35b806d32ef644ae3b5
Cr-Commit-Position: refs/heads/master@{#314514}