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

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

This's the Blink side patch to make ServiceWorkerContainer
.ready property more align with the current spec.

Before this change, "ready" could be resolved before user
access it. After this change, "ready" property promise will
only be created the first time accessed. And the request
will be handled by browser process as the associated
registration might not be the matched registration for
"ready".

This means now "ready" maybe resolved with a later
registered longer matched registration other than the
registration associated when navigation.

Chrome CL: https://codereview.chromium.org/894973003/
Test: https://codereview.chromium.org/912443002/

BUG=454726
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191183

[xiang, 2015-02-03 09:15:32]

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

[xiang, 2015-02-03 09:17:33]

Hi, would you take a look of this? I think this's an issue, if you don't want to
change it now, please let me know, thanks!

[falken, 2015-02-09 01:47:08]

Dominic should have a look at this as I remember there was some trickiness to
getting ready return a "stable promise" which this patch might affect.

I don't have a good idea of what the web-exposed behavior difference this patch
makes, from your description:
> Before this change, "ready" will be resolved before user
> access it. After this change, "ready" promise property will
> be created when first access."

Can you show a test case or sample of what the difference is?

[dominicc (has gone to gerrit), 2015-02-09 06:53:24]

On 2015/02/09 at 01:47:08, falken wrote:
> Dominic should have a look at this as I remember there was some trickiness to
getting ready return a "stable promise" which this patch might affect.

That aspect LGTM. The stable promises are handled by ReadyPromise, and how it is
being used here LGTM.

However, could you add back the assertions that you removed in this patch? Those
assertions should still apply, I think?

> I don't have a good idea of what the web-exposed behavior difference this
patch makes, from your description:
> > Before this change, "ready" will be resolved before user
> > access it. After this change, "ready" promise property will
> > be created when first access."
> 
> Can you show a test case or sample of what the difference is?

I don't think this is user-visible behavior. It seems to be changing from
eagerly setting the 'ready' property, to retrieving it lazily from
m_provider->getReadyRegistration. A high-level comment about why we're making
this change would be helpful.

[xiang, 2015-02-09 08:00:59]

New patchsets have been uploaded after l-g-t-m from dominicc@chromium.org

[xiang, 2015-02-09 08:29:57]

Thanks for the reviews!

On 2015/02/09 06:53:24, dominicc wrote:
> On 2015/02/09 at 01:47:08, falken wrote:
> > Dominic should have a look at this as I remember there was some trickiness
to
> getting ready return a "stable promise" which this patch might affect.
> 
> That aspect LGTM. The stable promises are handled by ReadyPromise, and how it
is
> being used here LGTM.
> 
> However, could you add back the assertions that you removed in this patch?
Those
> assertions should still apply, I think?
> 

ASSERT(m_readyRegistration == readyRegistration) assertion is not valid anymore,
as the m_readyRegistration no longer exist.
I added ASSERT(m_ready->state() == ReadyProperty::Pending) assertion on the
callback.

> > I don't have a good idea of what the web-exposed behavior difference this
> patch makes, from your description:
> > > Before this change, "ready" will be resolved before user
> > > access it. After this change, "ready" promise property will
> > > be created when first access."
> > 
> > Can you show a test case or sample of what the difference is?

I uploaded the test CL at: https://codereview.chromium.org/912443002/, please
take a look.

> 
> I don't think this is user-visible behavior. It seems to be changing from
> eagerly setting the 'ready' property, to retrieving it lazily from
> m_provider->getReadyRegistration. A high-level comment about why we're making
> this change would be helpful.

I think it is user-visible. For example: an already controlled page accesses
"ready" when another longer matched registration registered.
In previous impl/spec, the "ready" prop will be resolved with the navigation
associated registration, now it will be resolved with the new longer matched
registration(if it can become active).

[dominicc (has gone to gerrit), 2015-02-10 02:46:07]

On 2015/02/09 at 08:29:57, xiang.long wrote:
> > I don't think this is user-visible behavior. It seems to be changing from
> > eagerly setting the 'ready' property, to retrieving it lazily from
> > m_provider->getReadyRegistration. A high-level comment about why we're
making
> > this change would be helpful.
> 
> I think it is user-visible. For example: an already controlled page accesses
"ready" when another longer matched registration registered.
> In previous impl/spec, the "ready" prop will be resolved with the navigation
associated registration, now it will be resolved with the new longer matched
registration(if it can become active).

Good point.

Here's the spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#navigator-se...

I think the spec needs to change, and this doesn't quite implement the spec.
Imagine you retrieve "ready" ten times in a loop before the active worker
changes (which settles a Promise). I think the spec says you'd create ten
separate promises:

1. If the context object's ready promise is null, then:
  1. Set the context object's ready promise to a new promise.
2. If the context object's ready promise is settled, then:
  1. Return the context object's ready promise.

Not only that; it's only the last promise that is settled, the other nine just
lose:

5.4 Resolve context object's ready promise with a ServiceWorkerRegistration
object,

Could you file a spec bug to clear this up? What you have implemented is better,
but I still have my doubts (the ready promise is only useful once; shouldn't the
result change when the registration changes or the active worker changes? For
that purpose it would be better to return a new promise each time, but actually
resolve them!)

[dominicc (has gone to gerrit), 2015-02-10 03:02:13]

On 2015/02/10 at 02:46:07, dominicc wrote:
> On 2015/02/09 at 08:29:57, xiang.long wrote:
> > > I don't think this is user-visible behavior. It seems to be changing from
> > > eagerly setting the 'ready' property, to retrieving it lazily from
> > > m_provider->getReadyRegistration. A high-level comment about why we're
making
> > > this change would be helpful.
> > 
> > I think it is user-visible. For example: an already controlled page accesses
"ready" when another longer matched registration registered.
> > In previous impl/spec, the "ready" prop will be resolved with the navigation
associated registration, now it will be resolved with the new longer matched
registration(if it can become active).
> 
> Good point.
> 
> Here's the spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#navigator-se...
> 
> I think the spec needs to change, and this doesn't quite implement the spec.
Imagine you retrieve "ready" ten times in a loop before the active worker
changes (which settles a Promise). I think the spec says you'd create ten
separate promises:
> 
> 1. If the context object's ready promise is null, then:
>   1. Set the context object's ready promise to a new promise.
> 2. If the context object's ready promise is settled, then:
>   1. Return the context object's ready promise.
> 
> Not only that; it's only the last promise that is settled, the other nine just
lose:
> 
> 5.4 Resolve context object's ready promise with a ServiceWorkerRegistration
object,
> 
> Could you file a spec bug to clear this up? What you have implemented is
better, but I still have my doubts (the ready promise is only useful once;
shouldn't the result change when the registration changes or the active worker
changes? For that purpose it would be better to return a new promise each time,
but actually resolve them!)

Actually, I wasn't reading the spec carefully enough--I thought it was resetting
the promise each time until it is settled, but that's not the case.

This LGTM.

[xiang, 2015-02-12 04:26:57]

New patchsets have been uploaded after l-g-t-m from dominicc@chromium.org

[falken, 2015-02-16 08:36:57]

This looks good to me too.

Can you update the CL description to include a high-level motivation for the
change, as you've explained in the CL comments?

[xiang, 2015-02-26 06:50:28]

On 2015/02/16 08:36:57, falken wrote:
> This looks good to me too.
> 
> Can you update the CL description to include a high-level motivation for the
> change, as you've explained in the CL comments?

Done.

[falken, 2015-03-02 01:29:50]

lgtm

[xiang, 2015-03-02 07:59:54]

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

[xiang, 2015-03-02 07:59:55]

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

[commit-bot: I haz the power, 2015-03-02 08:00:48]

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

[commit-bot: I haz the power, 2015-03-02 08:16:02]

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

[commit-bot: I haz the power, 2015-03-02 08:16:03]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[xiang, 2015-03-02 08:19:27]

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

[xiang, 2015-03-02 08:21:45]

On 2015/03/02 08:19:27, xiang wrote:
> mailto:xiang.long@intel.com changed reviewers:
> + mailto:tkent@chromium.org

kent@ would you please review the public/platform/ changes? Thanks.

[tkent, 2015-03-02 08:26:35]

On 2015/03/02 08:21:45, xiang wrote:
> On 2015/03/02 08:19:27, xiang wrote:
> > mailto:xiang.long@intel.com changed reviewers:
> > + mailto:tkent@chromium.org
> 
> kent@ would you please review the public/platform/ changes? Thanks.

Build failure is not acceptable :)

[xiang, 2015-03-03 07:12:10]

On 2015/03/02 08:26:35, tkent wrote:
> On 2015/03/02 08:21:45, xiang wrote:
> > On 2015/03/02 08:19:27, xiang wrote:
> > > mailto:xiang.long@intel.com changed reviewers:
> > > + mailto:tkent@chromium.org
> > 
> > kent@ would you please review the public/platform/ changes? Thanks.
> 
> Build failure is not acceptable :)

Seems the try bots are OK with this CL now, would you take a look? Thanks!
I brought back some old code to make the layout tests pass.

[tkent, 2015-03-03 08:09:26]

rs lgtm

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

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

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

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

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

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

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

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191183