ServiceWorker: Make '.ready' return a promise to be resolved with ServiceWorkerRegistration (1/3)

ServiceWorker: Make '.ready' return a promise to be resolved with ServiceWorkerRegistration (1/3)

This change makes navigator.serviceWorker.ready return a promise to be resolved
with ServiceWorkerRegistration object instead of ServiceWorker object.

Tests for '.ready' (ie. 'ready.html') are now disabled and those will be
re-enabled by the 3rd patch.

1) Blink: THIS PATCH
2) Chromium: https://codereview.chromium.org/477593007/
3) Blink: https://codereview.chromium.org/532653002/

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#service-worker-container


BUG=399533
TEST=run_webkit_tests.py --debug http/tests/serviceworker/

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

[nhiroki, 2014-09-02 10:30:52]

nhiroki@chromium.org changed reviewers:
+ horo@chromium.org, michaeln@chromium.org

[nhiroki, 2014-09-02 10:30:53]

Hi, can you review this? Thanks.

[michaeln, 2014-09-03 22:17:48]

lgtm

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainer.h (right):

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.h:83: virtual void
setReady(WebServiceWorkerRegistration*) OVERRIDE;
Should this method be named setRegistration or maybe setReadyRegistration.

Sry for the naming nit but conventions are not real clear to me in the brave new
world of promises that blend events and attributes, onready is more or less the
event and registration is more or less the attribute. The attribute name is more
meaningful to me at least.

[michaeln, 2014-09-03 22:27:29]

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:225: void
ServiceWorkerContainer::setReady(WebServiceWorkerRegistration* registration)
should there be an assertion here that registration has an active version?

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:232:
RefPtrWillBeRawPtr<ServiceWorkerRegistration> previousReadyRegistration =
m_readyRegistration;
is it ever expected that the previous reg is not null?

[nhiroki, 2014-09-04 08:55:59]

Thank you for reviewing. Can you check my reply in ServiceWorkerContainer.cpp?

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:225: void
ServiceWorkerContainer::setReady(WebServiceWorkerRegistration* registration)
On 2014/09/03 22:27:29, michaeln wrote:
> should there be an assertion here that registration has an active version?

Done.

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:232:
RefPtrWillBeRawPtr<ServiceWorkerRegistration> previousReadyRegistration =
m_readyRegistration;
On 2014/09/03 22:27:29, michaeln wrote:
> is it ever expected that the previous reg is not null?

I think the previous reg could be non-null, for example, in the following case:

// in "/foo/bar/index.html"
nav.sW.register('/foo/')
  .then(function(reg) {
       return wait_for_activated(reg.installing);
     })
  .then(function() {
       // nav.sW.ready should be set to |reg|.
       return nav.sW.register('/foo/bar/');
     })
  .then(function(new_reg) {
       return wait_for_activated(new_reg.installing);
     })
  .then(function() {
       // nav.sW.ready should be replaced with longer matched |new_reg|.
     });

(Please let me know if I'm missing something...)

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainer.h (right):

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.h:83: virtual void
setReady(WebServiceWorkerRegistration*) OVERRIDE;
On 2014/09/03 22:17:48, michaeln wrote:
> Should this method be named setRegistration or maybe setReadyRegistration.
> 
> Sry for the naming nit but conventions are not real clear to me in the brave
new
> world of promises that blend events and attributes, onready is more or less
the
> event and registration is more or less the attribute. The attribute name is
more
> meaningful to me at least.

Sounds reasonable. Replaced 'setReady' with 'setReadyRegistration'.

[nhiroki, 2014-09-04 10:45:10]

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:232:
RefPtrWillBeRawPtr<ServiceWorkerRegistration> previousReadyRegistration =
m_readyRegistration;
On 2014/09/04 08:55:58, nhiroki wrote:
> On 2014/09/03 22:27:29, michaeln wrote:
> > is it ever expected that the previous reg is not null?
> 
> I think the previous reg could be non-null, for example, in the following
case:
> 
> // in "/foo/bar/index.html"
> nav.sW.register('/foo/')
>   .then(function(reg) {
>        return wait_for_activated(reg.installing);
>      })
>   .then(function() {
>        // nav.sW.ready should be set to |reg|.
>        return nav.sW.register('/foo/bar/');
>      })
>   .then(function(new_reg) {
>        return wait_for_activated(new_reg.installing);
>      })
>   .then(function() {
>        // nav.sW.ready should be replaced with longer matched |new_reg|.
>      });
> 
> (Please let me know if I'm missing something...)

Ah, you're right! The previous reg should always be null because the
browser-side doesn't allow the provider host to get associated with a new
registration when the host has already been associated with another one. I'll
fix this.

[nhiroki, 2014-09-04 13:06:57]

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/476043002/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:232:
RefPtrWillBeRawPtr<ServiceWorkerRegistration> previousReadyRegistration =
m_readyRegistration;
On 2014/09/04 10:45:10, nhiroki wrote:
> On 2014/09/04 08:55:58, nhiroki wrote:
> > On 2014/09/03 22:27:29, michaeln wrote:
> > > is it ever expected that the previous reg is not null?
> > 
> > I think the previous reg could be non-null, for example, in the following
> case:
> > 
> > // in "/foo/bar/index.html"
> > nav.sW.register('/foo/')
> >   .then(function(reg) {
> >        return wait_for_activated(reg.installing);
> >      })
> >   .then(function() {
> >        // nav.sW.ready should be set to |reg|.
> >        return nav.sW.register('/foo/bar/');
> >      })
> >   .then(function(new_reg) {
> >        return wait_for_activated(new_reg.installing);
> >      })
> >   .then(function() {
> >        // nav.sW.ready should be replaced with longer matched |new_reg|.
> >      });
> > 
> > (Please let me know if I'm missing something...)
> 
> Ah, you're right! The previous reg should always be null because the
> browser-side doesn't allow the provider host to get associated with a new
> registration when the host has already been associated with another one. I'll
> fix this.

Done. Removed checkReadyChanged() and added ASSERTs instead.

[nhiroki, 2014-09-04 13:11:00]

nhiroki@chromium.org changed reviewers:
+ tkent@chromium.org

[nhiroki, 2014-09-04 13:11:01]

+tkent@: Can you review this as an owner of public/platform? Thanks.

[horo, 2014-09-04 13:25:30]

lgtm

[michaeln, 2014-09-04 22:48:57]

lgtm 2

[tkent, 2014-09-05 02:01:25]

https://codereview.chromium.org/476043002/diff/190001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainer.h (right):

https://codereview.chromium.org/476043002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.h:98:
RefPtrWillBeMember<ServiceWorkerRegistration> m_readyRegistration;
Please update ServiceWorkerContainer::trace().

[nhiroki, 2014-09-05 02:23:05]

https://codereview.chromium.org/476043002/diff/190001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainer.h (right):

https://codereview.chromium.org/476043002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.h:98:
RefPtrWillBeMember<ServiceWorkerRegistration> m_readyRegistration;
On 2014/09/05 02:01:25, tkent wrote:
> Please update ServiceWorkerContainer::trace().

Done. Thanks!

[tkent, 2014-09-05 02:30:54]

lgtm

[nhiroki, 2014-09-05 04:06:14]

Thank you for reviewing!

FYI: Rebased on the CL to ship Oilpan for serviceworkers/
(https://codereview.chromium.org/478693005/). I'm going to check the CQ box when
tryservers look satisfied with the latest patchset.

[nhiroki, 2014-09-05 05:10:10]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-09-05 05:10:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/476043002/230001

[commit-bot: I haz the power, 2014-09-05 05:16:44]

Committed patchset #6 (id:230001) as 181435