Service Worker: Add ServiceWorkerContainer.getRegistrations() method.

Service Worker: Add ServiceWorkerContainer.getRegistrations() method.

Implement ServiceWorkerContainer.getReistrations() method as per spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#navigator-service-worker-getRegistrations

Companion CL: https://codereview.chromium.org/1146913004/
BUG=478382

[jungkees, 2015-05-21 05:17:50]

jungkee.song@samsung.com changed reviewers:
+ falken@chromium.org, kinuko@chromium.org, nhiroki@chromium.org

[jungkees, 2015-05-21 05:18:22]

PTAL

[falken, 2015-05-25 00:43:02]

Thanks for working on this! You can use BUG=478382

https://codereview.chromium.org/1143293003/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/getregistrations.html (right):

https://codereview.chromium.org/1143293003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistrations.html:5: <script>
This will be racy since async tests including promise_test run in parallel, and
getRegistrations() returns all the registrations of the origin. You're going to
need to either put each test in a single file, or use a trick like
sequential_promise_test (see fetch-test-helpers.js), so the tests do not affect
each other.

https://codereview.chromium.org/1143293003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistrations.html:15:
.catch(unreached_rejection(t));
for a promise_test, you only need to return a promise, no need to do t.done() or
catch().

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

https://codereview.chromium.org/1143293003/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:223:
ASSERT(RuntimeEnabledFeatures::serviceWorkerEnabled());
We should not add this, since we'd like to remove the serviceWorkerEnabled()
function.

[jungkees, 2015-05-27 08:06:46]

https://codereview.chromium.org/1143293003/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/getregistrations.html (right):

https://codereview.chromium.org/1143293003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistrations.html:5: <script>
On 2015/05/25 00:43:02, falken wrote:
> This will be racy since async tests including promise_test run in parallel,
and
> getRegistrations() returns all the registrations of the origin. You're going
to
> need to either put each test in a single file, or use a trick like
> sequential_promise_test (see fetch-test-helpers.js), so the tests do not
affect
> each other.

I gave it a try with sequential_promise_test, but it seems it still has the same
issue as 2 content_shells are running in parallel anyway. Is there anything I
missed to run it absolutely serialized?
When the test script runs only this file, it passes.

[jungkees, 2015-05-27 08:08:50]

Meantime, I'm having trouble uploading an updated patch set for the companion
CL: https://codereview.chromium.org/1146913004/. Will get it uploaded soon.

[falken, 2015-05-27 08:11:16]

On 2015/05/27 08:06:46, jungkees wrote:
>
https://codereview.chromium.org/1143293003/diff/20001/LayoutTests/http/tests/...
> File LayoutTests/http/tests/serviceworker/getregistrations.html (right):
> 
>
https://codereview.chromium.org/1143293003/diff/20001/LayoutTests/http/tests/...
> LayoutTests/http/tests/serviceworker/getregistrations.html:5: <script>
> On 2015/05/25 00:43:02, falken wrote:
> > This will be racy since async tests including promise_test run in parallel,
> and
> > getRegistrations() returns all the registrations of the origin. You're going
> to
> > need to either put each test in a single file, or use a trick like
> > sequential_promise_test (see fetch-test-helpers.js), so the tests do not
> affect
> > each other.
> 
> I gave it a try with sequential_promise_test, but it seems it still has the
same
> issue as 2 content_shells are running in parallel anyway. Is there anything I
> missed to run it absolutely serialized?
> When the test script runs only this file, it passes.

The 2 content shells run in parallel but their storage is isolated from each
other, so I wouldn't expect a conflict to occur.

[jungkees, 2015-06-03 12:46:40]

On 2015/05/27 08:11:16, falken wrote:
> The 2 content shells run in parallel but their storage is isolated from each
> other, so I wouldn't expect a conflict to occur.

Yes it was not a multi-instance issue. I found two existing layout test files
that affect the getRegistrations() behavior:
- claim-not-using-registration.html
  This script had registered 'wait-forever-in-install-worker.js'. I changed it
to using 'empty-worker.js' for the longer matched registration and unregistered
this registration at the end of the sequence.
- fetch-event-after-navigation-within-page.html
  Updated this script with sequential_promise_test()

The layout test is now successfully done. PTAL.

[falken, 2015-06-05 01:36:39]

Just looked at tests this time.

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/claim-not-using-registration.html
(right):

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:11:
sequential_promise_test(function(t) {
It's not clear to me why sequential_promise_test is needed here. The scopes are
different so the tests shouldn't collide. Can you explain?

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:82: var
installing_worker_url = 'resources/empty-worker.js';
Xiang: Do you remember why you used wait-forever-in-install-worker.js for this?
If we want to test claim() for the case of an installing worker, we should add a
way to let the worker cleanly unregister, e..g, postMessage to break the waiting
forever in install.

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:124:
sequential_promise_test_done();
Nit: Easier to read with a newline before this.

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/fetch-event-after-navigation-within-page.html
(right):

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-event-after-navigation-within-page.html:6:
<script src="../fetch/resources/fetch-test-helpers.js"></script>
Can you explain why we need sequential_promise_test for this particular file?

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-event-after-navigation-within-page.html:65:
sequential_promise_test_done();
nit: newline above this

[falken, 2015-06-05 01:39:47]

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

[falken, 2015-06-05 01:39:48]

+Xiang: See above comment about wait-forever-in-installing-worker.js

[jungkees, 2015-06-05 02:08:13]

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/claim-not-using-registration.html
(right):

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:11:
sequential_promise_test(function(t) {
While I'd tried quite a few different fixes, I left it this way. Just tested
it's working fine with promise_test and changed it back as such.

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:124:
sequential_promise_test_done();
Resolved by removing this call.

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/fetch-event-after-navigation-within-page.html
(right):

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-event-after-navigation-within-page.html:6:
<script src="../fetch/resources/fetch-test-helpers.js"></script>
As you pointed, it shouldn't have to be using sequential_promise_test. I changed
it back to using promise_test. But I left the return statements for
service_worker_unregister_and_register() promises; otherwise they end up with
getRegistrations() error.

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-event-after-navigation-within-page.html:65:
sequential_promise_test_done();
Resolved by removing this call.

[jungkees, 2015-06-05 02:35:57]

Just uploaded a new snapshot removing an unused param in
claim-not-using-registration.html. PTAL.

[xiang, 2015-06-08 02:55:29]

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/claim-not-using-registration.html
(right):

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:82: var
installing_worker_url = 'resources/empty-worker.js';
On 2015/06/05 01:36:39, falken wrote:
> Xiang: Do you remember why you used wait-forever-in-install-worker.js for
this?
> If we want to test claim() for the case of an installing worker, we should add
a
> way to let the worker cleanly unregister, e..g, postMessage to break the
waiting
> forever in install.

I intended to test claim() failure when there's a installing longer matched
registration. Yes, we should let the worker cleanly unregistered instead of fail
timeout or unregister until next registration.

And I feel this change is OK, as the longer matched registration might in state
of installing/activated... the claim() should fail on this client not matter the
worker in which state.

BTW, I'm not sure why we not support doom installing worker when new unregister
job pushed?

[falken, 2015-06-08 08:04:32]

Thanks for the update. I'll wait for the Chromium patch to get ready before
official stamp. Does the patch need to be 3-sided, or can it just be one
Chromium patch then one Blink patch?

And before I forget, let's make sure the patch is reviewed by an API OWNER
before landing. I don't think it needs an Intent to Ship, but at least an OWNER
should look.

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/claim-not-using-registration.html
(right):

https://codereview.chromium.org/1143293003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:82: var
installing_worker_url = 'resources/empty-worker.js';
On 2015/06/08 02:55:29, xiang wrote:
> On 2015/06/05 01:36:39, falken wrote:
> > Xiang: Do you remember why you used wait-forever-in-install-worker.js for
> this?
> > If we want to test claim() for the case of an installing worker, we should
add
> a
> > way to let the worker cleanly unregister, e..g, postMessage to break the
> waiting
> > forever in install.
> 
> I intended to test claim() failure when there's a installing longer matched
> registration. Yes, we should let the worker cleanly unregistered instead of
fail
> timeout or unregister until next registration.
> 
> And I feel this change is OK, as the longer matched registration might in
state
> of installing/activated... the claim() should fail on this client not matter
the
> worker in which state.
> 
> BTW, I'm not sure why we not support doom installing worker when new
unregister
> job pushed?

Thanks for the comment Xiang. So it sounds OK to proceed using empty-worker.js.

I think doom installing worker doesn't occur on unregister() because we want the
register/update/unregister jobs to be a simple FIFO queue of atomic jobs as much
as possible, having one job affect another causes complexity and bugs.

[jungkees, 2015-06-08 16:15:57]

On 2015/06/08 08:04:32, falken wrote:
> Thanks for the update. I'll wait for the Chromium patch to get ready before
> official stamp. Does the patch need to be 3-sided, or can it just be one
> Chromium patch then one Blink patch?
>

A new snapshot for the Chromium patch has been just uploaded. I don't think I
understood what the 3-sided option would be like. Could you explain?
 
> And before I forget, let's make sure the patch is reviewed by an API OWNER
> before landing. I don't think it needs an Intent to Ship, but at least an
OWNER
> should look.

Could you guide me what should be done? Should I find and ask the review to API
OWNER? Or you've just done it by having sent this message?

[falken, 2015-06-09 07:21:22]

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

[falken, 2015-06-09 07:21:25]

+tkent: for web/ OWNER (I think this doesn't require an Intent to Ship, the
addition is similar to an already shipped method getRegistration() [non-plural])

On 2015/06/08 16:15:57, jungkees wrote:
> On 2015/06/08 08:04:32, falken wrote:
> > Thanks for the update. I'll wait for the Chromium patch to get ready before
> > official stamp. Does the patch need to be 3-sided, or can it just be one
> > Chromium patch then one Blink patch?
> >
> 
> A new snapshot for the Chromium patch has been just uploaded. I don't think I
> understood what the 3-sided option would be like. Could you explain?

3-sided patches are a pain but sometimes necessary. You have to land the patches
in such a way that tip of tree is never broken. As is, your Chromium patch
depends on your Blink patch (for WebServiceWorkerProvider.h changes), yet your
Blink patch depends on your Chromium patch (to pass the layout tests). Therefore
landing either patch first would break the tree.

One way to solve this is to break the patches into three:
[1] Blink-side without layout tests
[2] Chromium-side
[3] Blink-side layout tests

Now you can land [1] then [2] (once the Blink version used by Chromium includes
[1], see the DEPS file for this), then [3], and at no time will the tree be
broken.



>  
> > And before I forget, let's make sure the patch is reviewed by an API OWNER
> > before landing. I don't think it needs an Intent to Ship, but at least an
> OWNER
> > should look.
> 
> Could you guide me what should be done? Should I find and ask the review to
API
> OWNER? Or you've just done it by having sent this message?

Adding tkent.

[tkent, 2015-06-09 07:51:20]

On 2015/06/09 07:21:25, falken wrote:
> > > And before I forget, let's make sure the patch is reviewed by an API OWNER
> > > before landing. I don't think it needs an Intent to Ship, but at least an
> > OWNER
> > > should look.
> > 
> > Could you guide me what should be done? Should I find and ask the review to
> API
> > OWNER? Or you've just done it by having sent this message?
> 
> Adding tkent.

Does Service Worker team assume this is a trivial change?
If so, it's ok to proceed this without Intent to Ship.  However I recommend to
make a chromestatus.com entry anyway.

[falken, 2015-06-09 08:03:09]

On 2015/06/09 07:51:20, tkent wrote:
> On 2015/06/09 07:21:25, falken wrote:
> > > > And before I forget, let's make sure the patch is reviewed by an API
OWNER
> > > > before landing. I don't think it needs an Intent to Ship, but at least
an
> > > OWNER
> > > > should look.
> > > 
> > > Could you guide me what should be done? Should I find and ask the review
to
> > API
> > > OWNER? Or you've just done it by having sent this message?
> > 
> > Adding tkent.
> 
> Does Service Worker team assume this is a trivial change?
> If so, it's ok to proceed this without Intent to Ship.  However I recommend to
> make a http://chromestatus.com entry anyway.

Yeah, I consider this change trivial.
(the API change, not the coding! Thanks for the patch Jungkee)

[jungkees, 2015-06-09 08:20:56]

Okay, I'll make it a 3-sided patch as suggested.

[falken, 2015-06-10 03:22:24]

On 2015/06/09 08:20:56, jungkees wrote:
> Okay, I'll make it a 3-sided patch as suggested.

Thanks, let me close this one so reviews don't get confused.

[jungkees, 2015-06-10 03:56:25]

Sure. Thanks.