ServiceWorker: Make "ready" fetches registration from browser process(2/3).

ServiceWorker: Make "ready" fetches registration from browser process(2/3).

ServiceWorkerProviderHost will maintain associated
registration and all matching living registrations during
its lifetime. Get registration for "ready" property request
will be handled by fetching the longest matched one without
hitting the DB.

The MatchRegistration() method could also be used by
"claim" functionality later.

Blink CL: https://codereview.chromium.org/894983002/
Cleanup & Test: https://codereview.chromium.org/912443002/

BUG=454726, 454250, 462529
Committed: https://crrev.com/71a34b78703ad8bfefbd8c54ea78414e8cda7eca
Cr-Commit-Position: refs/heads/master@{#319815}

[xiang, 2015-02-04 05:30:52]

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

[xiang, 2015-02-09 07:11:11]

Patchset #2 (id:20001) has been deleted

[xiang, 2015-02-09 08:36:12]

PTAL, thanks!

[xiang, 2015-02-09 08:50:21]

Patchset #2 (id:40001) has been deleted

[xiang, 2015-02-10 01:45:58]

Patchset #3 (id:80001) has been deleted

[dominicc (has gone to gerrit), 2015-02-10 04:10:54]

Some comments inline.

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

https://codereview.chromium.org/894973003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:162: // Calls by
dispatcher host to get "ready" registration.
Calls -> Called

"ready" registration -> the registration for the "ready" property

Would it make sense to link to the spec for ready?

https://codereview.chromium.org/894973003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:196:
ServiceWorkerRegistration* GetMatchedRegistration() const;
I think it would be helpful to reference
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#scope-match-...
here, or on the implementation of the algorithm.

https://codereview.chromium.org/894973003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:244: // Contains
associated registration and matched registartions created
Spelling: registrations

I think this is too confusing, we have

associated registrations
potential registrations
matched registrations
and ready registrations

I think you need to pick very distinct words and establish their separate
meaning with slightly more detailed comments.

If possible it would be good to align vocabulary with the spec (match,
matching). Instead of "ready registration" you could name things like
"registration for ready" perhaps.

[xiang, 2015-02-12 07:12:52]

Patchset #4 (id:120001) has been deleted

[xiang, 2015-02-12 07:14:23]

Thanks for the review, CL updated. I also cleaned & added new tests for sw
provider host. Would you please take another look?

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

https://codereview.chromium.org/894973003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:162: // Calls by
dispatcher host to get "ready" registration.
On 2015/02/10 04:10:53, dominicc wrote:
> Calls -> Called
> 
> "ready" registration -> the registration for the "ready" property
> 
> Would it make sense to link to the spec for ready?

Done.

https://codereview.chromium.org/894973003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:196:
ServiceWorkerRegistration* GetMatchedRegistration() const;
On 2015/02/10 04:10:54, dominicc wrote:
> I think it would be helpful to reference
>
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#scope-match-...
> here, or on the implementation of the algorithm.

Done. It's an optimized impl of the algorithm for a document, use cases like
getRegistration() should still hit the DB.

https://codereview.chromium.org/894973003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:244: // Contains
associated registration and matched registartions created
On 2015/02/10 04:10:53, dominicc wrote:
> Spelling: registrations
> 
> I think this is too confusing, we have
> 
> associated registrations
> potential registrations
> matched registrations
> and ready registrations
> 
> I think you need to pick very distinct words and establish their separate
> meaning with slightly more detailed comments.
> 
> If possible it would be good to align vocabulary with the spec (match,
> matching). Instead of "ready registration" you could name things like
> "registration for ready" perhaps.

Done, I tweaked these comments and names, please take another look.

[xiang, 2015-02-12 07:27:17]

Patchset #4 (id:140001) has been deleted

[dominicc (has gone to gerrit), 2015-02-16 07:07:15]

On 2015/02/12 at 07:27:17, xiang.long wrote:
> Patchset #4 (id:140001) has been deleted

This LGTM, but I'll ping falken to look at this too since he knows it 1000x
better than me.

[falken, 2015-02-16 09:18:29]

looking good to me too

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:513:
provider_host->GetRegistrationForReady(
GetRegistrationForReady does a DCHECK that it hasn't been called before, but a
misbehaving renderer will violate that assumption. Can we do a
BadMessageReceived() here in that case?

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

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:137:
DisassociateRegistration();
Shouldn't you RemoveMatchingRegistration in this case also?

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:274: break;
nit: maybe just return it->second.get() here? then the final return is just
nullptr.

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:391:
DCHECK(get_ready_callback_.is_null());
As in my other comment, this DCHECK can be violated if the renderer is
compromised. Instead maybe return false here, so the caller can can
BadMessageReceived.

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:576:
get_ready_callback_.Reset();
Are you sure we should reset it here? I think the GetRegistrationForReady should
only be called once per the lifetime of the document, no?

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

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:161: //
http://www.w3.org/TR/service-workers/#navigator-service-worker-ready.
nit: We always link to the latest editor's draft:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/

"TR" links can get outdated.

[xiang, 2015-02-26 06:52:15]

Thanks for the review! CL updated.

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:513:
provider_host->GetRegistrationForReady(
On 2015/02/16 09:18:29, falken wrote:
> GetRegistrationForReady does a DCHECK that it hasn't been called before, but a
> misbehaving renderer will violate that assumption. Can we do a
> BadMessageReceived() here in that case?

Done. I put it into GetRegistrationForReadyComplete().

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

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:137:
DisassociateRegistration();
On 2015/02/16 09:18:29, falken wrote:
> Shouldn't you RemoveMatchingRegistration in this case also?

You're right, done.

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:274: break;
On 2015/02/16 09:18:29, falken wrote:
> nit: maybe just return it->second.get() here? then the final return is just
> nullptr.

Done.

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:391:
DCHECK(get_ready_callback_.is_null());
On 2015/02/16 09:18:29, falken wrote:
> As in my other comment, this DCHECK can be violated if the renderer is
> compromised. Instead maybe return false here, so the caller can can
> BadMessageReceived.

Done.

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:576:
get_ready_callback_.Reset();
On 2015/02/16 09:18:29, falken wrote:
> Are you sure we should reset it here? I think the GetRegistrationForReady
should
> only be called once per the lifetime of the document, no?

Yes, but I thought we already made Blink can only call this once for each
document.
Not resetting it maybe more safe.

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

https://codereview.chromium.org/894973003/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:161: //
http://www.w3.org/TR/service-workers/#navigator-service-worker-ready.
On 2015/02/16 09:18:29, falken wrote:
> nit: We always link to the latest editor's draft:
> https://slightlyoff.github.io/ServiceWorker/spec/service_worker/
> 
> "TR" links can get outdated.

Done.

[michaeln, 2015-02-26 21:41:06]

drive by: this change looks great

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

https://codereview.chromium.org/894973003/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:86:
callback.Reset();
not sure this line is needed?

[xiang, 2015-02-27 09:33:46]

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

https://codereview.chromium.org/894973003/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:86:
callback.Reset();
On 2015/02/26 21:41:06, michaeln wrote:
> not sure this line is needed?

Done. This's not needed.

[falken, 2015-03-02 02:09:41]

lgtm

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:508:
BadMessageReceived();
It seems possible for IsContextAlive to be false here if db corruption recovery
occurred: https://codereview.chromium.org/325173002

Other message handlers in this class just return error or return in that case.

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

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:167: //
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#navigator-se...
Can you add a comment about the return value?

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host_unittest.cc
(right):

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:80: //
Add a same registration twice has no effect.
nit: "Add a" -> "Adding the"

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:84: //
Different matching registrations could be added.
nit: "could" -> "can"

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:88: //
Remove matching registration will decrease the process refs for its
nit: "Remove" -> "Removing a"

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:93: //
Multiple provider hosts could add the same matching registration.
nit: "could" -> "can"

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:103: //
Associate the registration will also increases the process refs for
nit: "Associate" -> "Associating", "increases" -> "increase"

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:108: //
Disassociate the registration shouldn't effect the process refs for
nit: "Disassociate" -> "Disassociating", "effect" -> "affect"

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:123: //
Should return nullptr after remove all matching registrations.
nit: "remove" -> "removing"

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:263:
NotifyProviderHostsForRegistration(registration());
nit: By the name only, I'd wonder what this function notifies provider hosts
about. AddRegistrationToMatchingProviderHosts is a more direct name for me.

[xiang, 2015-03-03 08:56:04]

Thanks for the review.

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:508:
BadMessageReceived();
On 2015/03/02 02:09:40, falken wrote:
> It seems possible for IsContextAlive to be false here if db corruption
recovery
> occurred: https://codereview.chromium.org/325173002
> 
> Other message handlers in this class just return error or return in that case.

Done.

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

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:167: //
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#navigator-se...
On 2015/03/02 02:09:40, falken wrote:
> Can you add a comment about the return value?

Done.

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host_unittest.cc
(right):

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:80: //
Add a same registration twice has no effect.
On 2015/03/02 02:09:41, falken wrote:
> nit: "Add a" -> "Adding the"

Done.

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:84: //
Different matching registrations could be added.
On 2015/03/02 02:09:41, falken wrote:
> nit: "could" -> "can"

Done.

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:88: //
Remove matching registration will decrease the process refs for its
On 2015/03/02 02:09:41, falken wrote:
> nit: "Remove" -> "Removing a"

Done.

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:93: //
Multiple provider hosts could add the same matching registration.
On 2015/03/02 02:09:41, falken wrote:
> nit: "could" -> "can"

Done.

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:103: //
Associate the registration will also increases the process refs for
On 2015/03/02 02:09:41, falken wrote:
> nit: "Associate" -> "Associating", "increases" -> "increase"

Done.

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:108: //
Disassociate the registration shouldn't effect the process refs for
On 2015/03/02 02:09:41, falken wrote:
> nit: "Disassociate" -> "Disassociating", "effect" -> "affect"

Done.

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:123: //
Should return nullptr after remove all matching registrations.
On 2015/03/02 02:09:41, falken wrote:
> nit: "remove" -> "removing"

Done.

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/894973003/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:263:
NotifyProviderHostsForRegistration(registration());
On 2015/03/02 02:09:41, falken wrote:
> nit: By the name only, I'd wonder what this function notifies provider hosts
> about. AddRegistrationToMatchingProviderHosts is a more direct name for me.

Agree, that's better.

[xiang, 2015-03-04 01:49:28]

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

[xiang, 2015-03-04 01:51:04]

On 2015/03/04 01:49:28, xiang wrote:
> mailto:xiang.long@intel.com changed reviewers:
> + mailto:mkwst@chromium.org

mkwst@ would you please review the
IPC(content/common/service_worker/service_worker_messages.h) change? Thanks!

[Mike West, 2015-03-04 08:34:54]

IPC LGTM.

[xiang, 2015-03-04 08:57:10]

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

[xiang, 2015-03-04 08:57:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominicc@chromium.org, falken@chromium.org
Link to the patchset: https://codereview.chromium.org/894973003/#ps240001
(title: "#15")

[commit-bot: I haz the power, 2015-03-04 08:57:45]

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

[commit-bot: I haz the power, 2015-03-04 13:12:50]

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

[commit-bot: I haz the power, 2015-03-04 13:12:52]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xiang, 2015-03-05 08:51:43]

falken@ I made some changes after your l-g-t-m, you may want to take another
look.
1. push messaging browser test uses "ready" which is broken until this CL
landed, so I added a work around helper;
2. there is a race condition that get ready complete IPC sent before
registration handle attributes change IPC. I added a new "OnActivateVersion"
listener method to make sure the ready complete IPC be sent later.

[falken, 2015-03-05 09:21:52]

https://codereview.chromium.org/894973003/diff/260001/chrome/test/data/push_m...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/894973003/diff/260001/chrome/test/data/push_m...
chrome/test/data/push_messaging/push_test.js:95: // TODO(xiang): Remove this
function after "ready" CL landed.
I'm confused, I thought *this* is the "ready" CL? How does this CL break
navigator.serviceWorker.ready?

https://codereview.chromium.org/894973003/diff/260001/content/browser/service...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/894973003/diff/260001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:50: virtual void
OnActivateVersion(ServiceWorkerRegistration* registration) {}
It's kind of bizarre to have both OnVersionAttributesChanged and
OnActivateVersion. Is there a cleaner way to get the IPC ordering we want?

[xiang, 2015-03-06 07:18:41]

On 2015/03/05 09:21:52, falken wrote:
>
https://codereview.chromium.org/894973003/diff/260001/chrome/test/data/push_m...
> File chrome/test/data/push_messaging/push_test.js (right):
> 
>
https://codereview.chromium.org/894973003/diff/260001/chrome/test/data/push_m...
> chrome/test/data/push_messaging/push_test.js:95: // TODO(xiang): Remove this
> function after "ready" CL landed.
> I'm confused, I thought *this* is the "ready" CL? How does this CL break
> navigator.serviceWorker.ready?
> 

Sorry， I meant the followup test&clean CL, not this one.
In https://codereview.chromium.org/894983002/ I kept some old code to make the
layout tests pass(not the ready.html which has been disabled temporarily, other
tests will trigger DCHECK failure as the
ServiceWorkerDispatcher::SetReadyRegistration function is not removed yet). So
the "ready" remains broken until next CL landed.

>
https://codereview.chromium.org/894973003/diff/260001/content/browser/service...
> File content/browser/service_worker/service_worker_registration.h (right):
> 
>
https://codereview.chromium.org/894973003/diff/260001/content/browser/service...
> content/browser/service_worker/service_worker_registration.h:50: virtual void
> OnActivateVersion(ServiceWorkerRegistration* registration) {}
> It's kind of bizarre to have both OnVersionAttributesChanged and
> OnActivateVersion. Is there a cleaner way to get the IPC ordering we want?

How about change a more explicit name?
I tried to call the ReturnRegistrationForReadyIfNeeded() in the end of
SendSetVersionAttributesMessage(), however noticed that the provider wait for
"ready" maybe not always has a registration handle listening on the event.
Or I can copy some OnSetVersionAttributes() logic into
OnDidGetRegistrationForReady(), however that will also complicate the code.

[falken, 2015-03-06 08:26:17]

On 2015/03/06 07:18:41, xiang wrote:
> On 2015/03/05 09:21:52, falken wrote:
> >
>
https://codereview.chromium.org/894973003/diff/260001/chrome/test/data/push_m...
> > File chrome/test/data/push_messaging/push_test.js (right):
> > 
> >
>
https://codereview.chromium.org/894973003/diff/260001/chrome/test/data/push_m...
> > chrome/test/data/push_messaging/push_test.js:95: // TODO(xiang): Remove this
> > function after "ready" CL landed.
> > I'm confused, I thought *this* is the "ready" CL? How does this CL break
> > navigator.serviceWorker.ready?
> > 
> 
> Sorry， I meant the followup test&clean CL, not this one.
> In https://codereview.chromium.org/894983002/ I kept some old code to make the
> layout tests pass(not the ready.html which has been disabled temporarily,
other
> tests will trigger DCHECK failure as the
> ServiceWorkerDispatcher::SetReadyRegistration function is not removed yet). So
> the "ready" remains broken until next CL landed.

Ah I see, so the follow up CL to this one is not just:
https://codereview.chromium.org/912443002/ 

> 
> >
>
https://codereview.chromium.org/894973003/diff/260001/content/browser/service...
> > File content/browser/service_worker/service_worker_registration.h (right):
> > 
> >
>
https://codereview.chromium.org/894973003/diff/260001/content/browser/service...
> > content/browser/service_worker/service_worker_registration.h:50: virtual
void
> > OnActivateVersion(ServiceWorkerRegistration* registration) {}
> > It's kind of bizarre to have both OnVersionAttributesChanged and
> > OnActivateVersion. Is there a cleaner way to get the IPC ordering we want?
> 
> How about change a more explicit name?
> I tried to call the ReturnRegistrationForReadyIfNeeded() in the end of
> SendSetVersionAttributesMessage(), however noticed that the provider wait for
> "ready" maybe not always has a registration handle listening on the event.
> Or I can copy some OnSetVersionAttributes() logic into
> OnDidGetRegistrationForReady(), however that will also complicate the code.

Can you explain more why OnActivateVersion is needed? In the patch, you fire
OnActivateVersion after SetActiveVersion. I'd expect SetActiveVersion ->
SetVersionInternal to fire OnVersionAttributesChanged already.

[xiang, 2015-03-06 08:50:26]

On 2015/03/06 08:26:17, falken wrote:
> On 2015/03/06 07:18:41, xiang wrote:
> > On 2015/03/05 09:21:52, falken wrote:
> > >
> >
>
https://codereview.chromium.org/894973003/diff/260001/chrome/test/data/push_m...
> > > File chrome/test/data/push_messaging/push_test.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/894973003/diff/260001/chrome/test/data/push_m...
> > > chrome/test/data/push_messaging/push_test.js:95: // TODO(xiang): Remove
this
> > > function after "ready" CL landed.
> > > I'm confused, I thought *this* is the "ready" CL? How does this CL break
> > > navigator.serviceWorker.ready?
> > > 
> > 
> > Sorry， I meant the followup test&clean CL, not this one.
> > In https://codereview.chromium.org/894983002/ I kept some old code to make
the
> > layout tests pass(not the ready.html which has been disabled temporarily,
> other
> > tests will trigger DCHECK failure as the
> > ServiceWorkerDispatcher::SetReadyRegistration function is not removed yet).
So
> > the "ready" remains broken until next CL landed.
> 
> Ah I see, so the follow up CL to this one is not just:
> https://codereview.chromium.org/912443002/ 

Yes, I will update it to fix all the FIXMEs.

> 
> > 
> > >
> >
>
https://codereview.chromium.org/894973003/diff/260001/content/browser/service...
> > > File content/browser/service_worker/service_worker_registration.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/894973003/diff/260001/content/browser/service...
> > > content/browser/service_worker/service_worker_registration.h:50: virtual
> void
> > > OnActivateVersion(ServiceWorkerRegistration* registration) {}
> > > It's kind of bizarre to have both OnVersionAttributesChanged and
> > > OnActivateVersion. Is there a cleaner way to get the IPC ordering we want?
> > 
> > How about change a more explicit name?
> > I tried to call the ReturnRegistrationForReadyIfNeeded() in the end of
> > SendSetVersionAttributesMessage(), however noticed that the provider wait
for
> > "ready" maybe not always has a registration handle listening on the event.
> > Or I can copy some OnSetVersionAttributes() logic into
> > OnDidGetRegistrationForReady(), however that will also complicate the code.
> 
> Can you explain more why OnActivateVersion is needed? In the patch, you fire
> OnActivateVersion after SetActiveVersion. I'd expect SetActiveVersion ->
> SetVersionInternal to fire OnVersionAttributesChanged already.

OnVersionAttributesChanged listeners include ServiceWorkerRegistrationHandle and
ServiceWorkerRegistrationProviderHost, however the order matters.
If provider host is notified first, then "ready" promise may be resolved by a
registration in an intermediate state, because the "set version attrs" message
sent by ServiceWorkerRegistrationHandle is not handled yet. This happens when an
*existing* registration is found in
ServiceWorkerDispatcher::FindOrCreateRegistration, the passed attrs will be
discard, registration attrs will only updated in OnSetVersionAttributes.

[falken, 2015-03-06 09:07:49]

On 2015/03/06 08:50:26, xiang wrote:
> On 2015/03/06 08:26:17, falken wrote:
> > On 2015/03/06 07:18:41, xiang wrote:
> > > On 2015/03/05 09:21:52, falken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/894973003/diff/260001/chrome/test/data/push_m...
> > > > File chrome/test/data/push_messaging/push_test.js (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/894973003/diff/260001/chrome/test/data/push_m...
> > > > chrome/test/data/push_messaging/push_test.js:95: // TODO(xiang): Remove
> this
> > > > function after "ready" CL landed.
> > > > I'm confused, I thought *this* is the "ready" CL? How does this CL break
> > > > navigator.serviceWorker.ready?
> > > > 
> > > 
> > > Sorry， I meant the followup test&clean CL, not this one.
> > > In https://codereview.chromium.org/894983002/ I kept some old code to make
> the
> > > layout tests pass(not the ready.html which has been disabled temporarily,
> > other
> > > tests will trigger DCHECK failure as the
> > > ServiceWorkerDispatcher::SetReadyRegistration function is not removed
yet).
> So
> > > the "ready" remains broken until next CL landed.
> > 
> > Ah I see, so the follow up CL to this one is not just:
> > https://codereview.chromium.org/912443002/ 
> 
> Yes, I will update it to fix all the FIXMEs.
> 
> > 
> > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/894973003/diff/260001/content/browser/service...
> > > > File content/browser/service_worker/service_worker_registration.h
(right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/894973003/diff/260001/content/browser/service...
> > > > content/browser/service_worker/service_worker_registration.h:50: virtual
> > void
> > > > OnActivateVersion(ServiceWorkerRegistration* registration) {}
> > > > It's kind of bizarre to have both OnVersionAttributesChanged and
> > > > OnActivateVersion. Is there a cleaner way to get the IPC ordering we
want?
> > > 
> > > How about change a more explicit name?
> > > I tried to call the ReturnRegistrationForReadyIfNeeded() in the end of
> > > SendSetVersionAttributesMessage(), however noticed that the provider wait
> for
> > > "ready" maybe not always has a registration handle listening on the event.
> > > Or I can copy some OnSetVersionAttributes() logic into
> > > OnDidGetRegistrationForReady(), however that will also complicate the
code.
> > 
> > Can you explain more why OnActivateVersion is needed? In the patch, you fire
> > OnActivateVersion after SetActiveVersion. I'd expect SetActiveVersion ->
> > SetVersionInternal to fire OnVersionAttributesChanged already.
> 
> OnVersionAttributesChanged listeners include ServiceWorkerRegistrationHandle
and
> ServiceWorkerRegistrationProviderHost, however the order matters.
> If provider host is notified first, then "ready" promise may be resolved by a
> registration in an intermediate state, because the "set version attrs" message
> sent by ServiceWorkerRegistrationHandle is not handled yet. This happens when
an
> *existing* registration is found in
> ServiceWorkerDispatcher::FindOrCreateRegistration, the passed attrs will be
> discard, registration attrs will only updated in OnSetVersionAttributes.

Ah, I gotcha now. I think adding a new function like you've done may be OK, if
commented why it's needed. Another option is to not add the function and do:

ProviderHost::OnVersionAttributesChanged(registration) {
  if (new active version) {
    // Wait until the state change so we don't send the ready complete IPC until
after blah blah blah.
    active_version->AddListener(this);
}

ProviderHost::OnVersionStateChanged(version) {
  if (version is activating) {
    ReturnRegistrationForReadyIfNeeded()
    active_version->RemoveListener(this);
  }
}

WDYT?

[xiang, 2015-03-09 05:16:03]

> 
> Ah, I gotcha now. I think adding a new function like you've done may be OK, if
> commented why it's needed. Another option is to not add the function and do:
> 
> ProviderHost::OnVersionAttributesChanged(registration) {
>   if (new active version) {
>     // Wait until the state change so we don't send the ready complete IPC
until
> after blah blah blah.
>     active_version->AddListener(this);
> }
> 
> ProviderHost::OnVersionStateChanged(version) {
>   if (version is activating) {
>     ReturnRegistrationForReadyIfNeeded()
>     active_version->RemoveListener(this);
>   }
> }
> 
> WDYT?

That would be better, I used
registration->active_version()->RegisterStatusChangeCallback(), thanks!

[xiang, 2015-03-09 05:48:51]

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

[xiang, 2015-03-09 05:48:51]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
mkwst@chromium.org, dominicc@chromium.org
Link to the patchset: https://codereview.chromium.org/894973003/#ps280001
(title: "#30")

[commit-bot: I haz the power, 2015-03-09 05:49:56]

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

[falken, 2015-03-09 06:29:23]

lgtm

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

https://codereview.chromium.org/894973003/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:220:
OneShotGetReadyCallback(const GetRegistrationForReadyCallback& callback);
nit: explicit

https://codereview.chromium.org/894973003/diff/280001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher.h (right):

https://codereview.chromium.org/894973003/diff/280001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher.h:13: #include
"base/memory/scoped_ptr.h"
why add this?

[xiang, 2015-03-09 06:41:41]

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

[xiang, 2015-03-09 06:46:13]

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

https://codereview.chromium.org/894973003/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:220:
OneShotGetReadyCallback(const GetRegistrationForReadyCallback& callback);
On 2015/03/09 06:29:23, falken wrote:
> nit: explicit

Done.

https://codereview.chromium.org/894973003/diff/280001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher.h (right):

https://codereview.chromium.org/894973003/diff/280001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher.h:13: #include
"base/memory/scoped_ptr.h"
On 2015/03/09 06:29:23, falken wrote:
> why add this?

Done, forgot to remove it.

[xiang, 2015-03-09 06:46:19]

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

[xiang, 2015-03-09 06:46:20]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
mkwst@chromium.org, dominicc@chromium.org
Link to the patchset: https://codereview.chromium.org/894973003/#ps300001
(title: "#35")

[commit-bot: I haz the power, 2015-03-09 06:46:42]

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

[commit-bot: I haz the power, 2015-03-09 07:50:10]

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

[commit-bot: I haz the power, 2015-03-09 07:50:12]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-03-09 07:50:18]

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

[xiang, 2015-03-09 08:28:39]

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

[commit-bot: I haz the power, 2015-03-09 08:28:53]

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

[commit-bot: I haz the power, 2015-03-09 08:29:33]

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

[commit-bot: I haz the power, 2015-03-09 08:29:34]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-03-09 08:29:38]

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

[xiang, 2015-03-09 08:47:19]

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

[xiang, 2015-03-09 08:47:20]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
mkwst@chromium.org, dominicc@chromium.org
Link to the patchset: https://codereview.chromium.org/894973003/#ps320001
(title: "rebase")

[commit-bot: I haz the power, 2015-03-09 08:47:34]

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

[commit-bot: I haz the power, 2015-03-09 10:38:41]

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

[commit-bot: I haz the power, 2015-03-09 10:38:44]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-03-09 10:38:51]

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

[xiang, 2015-03-10 01:50:12]

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

[commit-bot: I haz the power, 2015-03-10 01:50:23]

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

[commit-bot: I haz the power, 2015-03-10 01:52:11]

Committed patchset #12 (id:320001)

[commit-bot: I haz the power, 2015-03-10 01:53:21]

Patchset 12 (id:??) landed as
https://crrev.com/71a34b78703ad8bfefbd8c54ea78414e8cda7eca
Cr-Commit-Position: refs/heads/master@{#319815}