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

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

This's the layout tests CL for ServiceWorkerClients.claim() support.

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

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

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

[xiang, 2015-01-23 05:00:58]

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

[xiang, 2015-01-23 05:01:09]

PTAL, thanks.

[xiang, 2015-01-23 08:57:58]

Patchset #3 (id:40001) has been deleted

[jsbell, 2015-01-24 01:09:38]

Given my understanding of the feature, lgtm with a few nits. And I'm not
understanding where the error message is coming from for the InvalidStateError -
did one of the other CLs change?

falken should also review.

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

https://codereview.chromium.org/872593002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:20:
.then(function() {
How about replacing lines 20...31 with:

.then(function() {
  return Promise.all([with_iframe(frame1_url), with_iframe(frame2_url)]);
})
.then(function(frames) {
  frame1 = frames[0];
  frame2 = frames[1];

https://codereview.chromium.org/872593002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:47: var
saw_controllerchanged = new Promise(function(resolve) {
Not necessarily for this CL but: we should really make a helper function like:

function promiseForEvent(target, type) {
  return new Promise(function(resolve) {
    target.addEventListener(type, resolve);
  }
}

So we can just write:

  var saw_controllerchanged = promiseForEvent(
      frame1.contentWindow.navigator.serviceworker, 'controllerchange');

And then below:

  var saw_message = promiseForEvent(channel.port1, 'message')
    .then(function(e) { assert_equals(e.data, 'PASS'); });

...but that could be a different CL.

https://codereview.chromium.org/872593002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:54:
assert_equals(e.data, 'PASS');
Can you add a message to this assertion? e.g. 'Worker call to claim() should
fulfill'

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

https://codereview.chromium.org/872593002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-using-registration.html:39:
assert_equals(e.data, 'PASS');
Can you add a message to this assertion? e.g. 'Worker call to claim() should
fulfill'

https://codereview.chromium.org/872593002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-using-registration.html:83:
assert_equals(e.data, 'FAIL: exception: InvalidStateError: ' +
Can you add a message to this assertion? e.g. 'Worker call to claim() should
reject with InvalidStateError'

https://codereview.chromium.org/872593002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-using-registration.html:84: 'Only
active worker can claim clients.');
Where is this message ("Only active worker...") coming from? I don't see the
string in http://crrev.com/868463004

[xiang, 2015-01-26 07:19:16]

Thanks for the review, CL updated. flaken@ would you please take a look? thanks.

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

https://codereview.chromium.org/872593002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:20:
.then(function() {
On 2015/01/24 01:09:38, jsbell wrote:
> How about replacing lines 20...31 with:
> 
> .then(function() {
>   return Promise.all([with_iframe(frame1_url), with_iframe(frame2_url)]);
> })
> .then(function(frames) {
>   frame1 = frames[0];
>   frame2 = frames[1];

Done. This looks better!

https://codereview.chromium.org/872593002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:47: var
saw_controllerchanged = new Promise(function(resolve) {
On 2015/01/24 01:09:38, jsbell wrote:
> Not necessarily for this CL but: we should really make a helper function like:
> 
> function promiseForEvent(target, type) {
>   return new Promise(function(resolve) {
>     target.addEventListener(type, resolve);
>   }
> }
> 
> So we can just write:
> 
>   var saw_controllerchanged = promiseForEvent(
>       frame1.contentWindow.navigator.serviceworker, 'controllerchange');
> 
> And then below:
> 
>   var saw_message = promiseForEvent(channel.port1, 'message')
>     .then(function(e) { assert_equals(e.data, 'PASS'); });
> 
> ...but that could be a different CL.

I can work on this later, thanks for the suggestion.

https://codereview.chromium.org/872593002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:54:
assert_equals(e.data, 'PASS');
On 2015/01/24 01:09:38, jsbell wrote:
> Can you add a message to this assertion? e.g. 'Worker call to claim() should
> fulfill'

Done.

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

https://codereview.chromium.org/872593002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-using-registration.html:39:
assert_equals(e.data, 'PASS');
On 2015/01/24 01:09:38, jsbell wrote:
> Can you add a message to this assertion? e.g. 'Worker call to claim() should
> fulfill'

Done.

https://codereview.chromium.org/872593002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-using-registration.html:83:
assert_equals(e.data, 'FAIL: exception: InvalidStateError: ' +
On 2015/01/24 01:09:38, jsbell wrote:
> Can you add a message to this assertion? e.g. 'Worker call to claim() should
> reject with InvalidStateError'

Done.

https://codereview.chromium.org/872593002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-using-registration.html:84: 'Only
active worker can claim clients.');
On 2015/01/24 01:09:38, jsbell wrote:
> Where is this message ("Only active worker...") coming from? I don't see the
> string in http://crrev.com/868463004
> 

The message defined in
https://codereview.chromium.org/871853002/patch/40001/50005.

[falken, 2015-01-27 03:25:54]

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

https://codereview.chromium.org/872593002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:16:
return service_worker_unregister_and_register(t, url1, frame2_url)
nit: "url1" and "url2" doesn't really speak to me, especially when "url1" is
paired with "frame2". if there's no better naming scheme at least can you rename
"url" to "worker_url"?

https://codereview.chromium.org/872593002/diff/80001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/claim-using-registration.html (right):

https://codereview.chromium.org/872593002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-using-registration.html:84:
assert_equals(e.data,'FAIL: exception: InvalidStateError: ' +
We shouldn't test chromium-specific error messages in w3c tests.

[xiang, 2015-01-28 07:36:44]

CL updated, thanks for the review!

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

https://codereview.chromium.org/872593002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:16:
return service_worker_unregister_and_register(t, url1, frame2_url)
On 2015/01/27 03:25:54, falken wrote:
> nit: "url1" and "url2" doesn't really speak to me, especially when "url1" is
> paired with "frame2". if there's no better naming scheme at least can you
rename
> "url" to "worker_url"?

Done.

https://codereview.chromium.org/872593002/diff/80001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/claim-using-registration.html (right):

https://codereview.chromium.org/872593002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/claim-using-registration.html:84:
assert_equals(e.data,'FAIL: exception: InvalidStateError: ' +
On 2015/01/27 03:25:54, falken wrote:
> We shouldn't test chromium-specific error messages in w3c tests.

Done.

[xiang, 2015-02-04 08:11:00]

Patchset #5 (id:100001) has been deleted

[xiang, 2015-02-04 08:14:27]

I added one more test for testing longer matched installing registration.
falken@ will you take another look of this? thanks.

[falken, 2015-02-05 10:53:05]

lgtm

Thanks for adding a test about the longer match.

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

https://codereview.chromium.org/872593002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:113:
'registration is installing');
as explained below, maybe more clear to say "when a longer matched registration
exists"?

https://codereview.chromium.org/872593002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:117: },
'Test claim client when a installing registration matches longer');
Maybe it's more clear to say "Test claim client when there's a longer-matching
registration not already used by the page". Whether or not the registration is
installing or installed isn't the big deal.

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

https://codereview.chromium.org/872593002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/claim-using-registration.html:97: }, 'Test
installed worker claim client which is using the same registration');
subjective nit: This is more readily understandable to me as "Test for the
waiting worker claims a client which is using the same registration"

[xiang, 2015-02-09 01:55:48]

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

[xiang, 2015-02-09 01:58:24]

Thanks for the review.

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

https://codereview.chromium.org/872593002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:113:
'registration is installing');
On 2015/02/05 10:53:05, falken wrote:
> as explained below, maybe more clear to say "when a longer matched
registration
> exists"?

Done.

https://codereview.chromium.org/872593002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/claim-not-using-registration.html:117: },
'Test claim client when a installing registration matches longer');
On 2015/02/05 10:53:05, falken wrote:
> Maybe it's more clear to say "Test claim client when there's a longer-matching
> registration not already used by the page". Whether or not the registration is
> installing or installed isn't the big deal.

Done.

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

https://codereview.chromium.org/872593002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/claim-using-registration.html:97: }, 'Test
installed worker claim client which is using the same registration');
On 2015/02/05 10:53:05, falken wrote:
> subjective nit: This is more readily understandable to me as "Test for the
> waiting worker claims a client which is using the same registration"

Done.

[xiang, 2015-02-09 01:58:39]

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

[commit-bot: I haz the power, 2015-02-09 01:58:58]

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

[xiang, 2015-02-09 02:36:04]

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

[xiang, 2015-02-09 02:41:28]

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

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

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

[commit-bot: I haz the power, 2015-02-09 03:54:14]

Committed patchset #7 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189777

[xiang, 2015-02-09 07:59:37]

Patchset #8 (id:180001) has been deleted