Implement ServiceWorkerClient attributes [3/3]

Implement ServiceWorkerClient attributes [3/3]

Patch dependency:
[1] Blink: https://crrev.com/783663002/
[2] Chromium: https://crrev.com/771103002/
[3] Blink: https://crrev.com/778703004/ (THIS PATCH)

Spec: http://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-client

BUG=436335, 437152
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186925

[Kunihiko Sakamoto, 2014-12-08 01:55:26]

Patchset #1 (id:1) has been deleted

[Kunihiko Sakamoto, 2014-12-08 02:01:45]

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

[nhiroki, 2014-12-08 06:02:47]

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/clients-getall.html (right):

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:8: var scope =
'resources/blank.html';
"blank.html?clients-getAll" could be more stable?

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:9: t.step(function() {
Optional: You may want to use 'promise_test' and omit '.catch'.

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:13: return
wait_for_update(t, registration);
To get a worker object, you can just use 'registration.installing' instead of
calling 'wait_for_update()' (see https://codereview.chromium.org/776373003/ for
more details)

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:35: ['visible', false,
new URL(scope + '#2', location).toString(), 'nested']
Can we have tests for other visibilityStates ('hidden' and 'preRender')?

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:42: t.done();
Calling service_worker_unregister_and_done() would be more polite.

https://codereview.chromium.org/778703004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerClient.h (right):

https://codereview.chromium.org/778703004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerClient.h:23: static
ServiceWorkerClient* create(const WebServiceWorkerClientInfo& into);
nit: s/into/info

[Kunihiko Sakamoto, 2014-12-08 07:38:31]

Patchset #2 (id:40001) has been deleted

[Kunihiko Sakamoto, 2014-12-08 07:40:00]

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/clients-getall.html (right):

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:8: var scope =
'resources/blank.html';
On 2014/12/08 06:02:47, nhiroki wrote:
> "blank.html?clients-getAll" could be more stable?

Done.

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:9: t.step(function() {
On 2014/12/08 06:02:47, nhiroki wrote:
> Optional: You may want to use 'promise_test' and omit '.catch'.

It didn't work as promise_test calls done() but we want to wait onMessage().

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:13: return
wait_for_update(t, registration);
On 2014/12/08 06:02:47, nhiroki wrote:
> To get a worker object, you can just use 'registration.installing' instead of
> calling 'wait_for_update()' (see https://codereview.chromium.org/776373003/
for
> more details)

Done.

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:35: ['visible', false,
new URL(scope + '#2', location).toString(), 'nested']
On 2014/12/08 06:02:46, nhiroki wrote:
> Can we have tests for other visibilityStates ('hidden' and 'preRender')?

We could call testRunner.setPageVisibility() but that will make this test
Chromium-specific.

I've made ServiceWorkerContainerTest cover the "hidden" case.

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:42: t.done();
On 2014/12/08 06:02:47, nhiroki wrote:
> Calling service_worker_unregister_and_done() would be more polite.

Done.

https://codereview.chromium.org/778703004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerClient.h (right):

https://codereview.chromium.org/778703004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerClient.h:23: static
ServiceWorkerClient* create(const WebServiceWorkerClientInfo& into);
On 2014/12/08 06:02:47, nhiroki wrote:
> nit: s/into/info

Removed this unnecessary parameter name.

[nhiroki, 2014-12-08 08:01:09]

lgtm

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/clients-getall.html (right):

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:9: t.step(function() {
On 2014/12/08 07:39:59, Kunihiko Sakamoto wrote:
> On 2014/12/08 06:02:47, nhiroki wrote:
> > Optional: You may want to use 'promise_test' and omit '.catch'.
> 
> It didn't work as promise_test calls done() but we want to wait onMessage().

I think wrapping onMessage() with Promise might work (This is optional and you
don't have to change the current style)

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:35: ['visible', false,
new URL(scope + '#2', location).toString(), 'nested']
On 2014/12/08 07:39:59, Kunihiko Sakamoto wrote:
> On 2014/12/08 06:02:46, nhiroki wrote:
> > Can we have tests for other visibilityStates ('hidden' and 'preRender')?
> 
> We could call testRunner.setPageVisibility() but that will make this test
> Chromium-specific.
> 
> I've made ServiceWorkerContainerTest cover the "hidden" case.

Acknowledged.

https://codereview.chromium.org/778703004/diff/60001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/clients-getall.html (right):

https://codereview.chromium.org/778703004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:25: });
';' is not necessary.

https://codereview.chromium.org/778703004/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp (right):

https://codereview.chromium.org/778703004/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp:387:
EXPECT_EQ(WebURLRequest::FrameTypeTopLevel, info.frameType);
How about calling setVisibilityState(Visible) and getClieintInfo() here?

[Kunihiko Sakamoto, 2014-12-08 08:34:10]

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

[Kunihiko Sakamoto, 2014-12-08 08:34:11]

+tkent for public/platform

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/clients-getall.html (right):

https://codereview.chromium.org/778703004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:9: t.step(function() {
On 2014/12/08 08:01:08, nhiroki wrote:
> On 2014/12/08 07:39:59, Kunihiko Sakamoto wrote:
> > On 2014/12/08 06:02:47, nhiroki wrote:
> > > Optional: You may want to use 'promise_test' and omit '.catch'.
> > 
> > It didn't work as promise_test calls done() but we want to wait onMessage().
> 
> I think wrapping onMessage() with Promise might work (This is optional and you
> don't have to change the current style)

OK, let me keep the current style as it's easier to understand for me.

https://codereview.chromium.org/778703004/diff/60001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/clients-getall.html (right):

https://codereview.chromium.org/778703004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/clients-getall.html:25: });
On 2014/12/08 08:01:09, nhiroki wrote:
> ';' is not necessary.

Oops, done.

https://codereview.chromium.org/778703004/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp (right):

https://codereview.chromium.org/778703004/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp:387:
EXPECT_EQ(WebURLRequest::FrameTypeTopLevel, info.frameType);
On 2014/12/08 08:01:09, nhiroki wrote:
> How about calling setVisibilityState(Visible) and getClieintInfo() here?

Done.

[tkent, 2014-12-08 08:39:23]

lgtm

[mlamouri (slow - plz ping), 2014-12-08 10:41:18]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-12-08 10:41:19]

https://codereview.chromium.org/778703004/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerClient.idl (right):

https://codereview.chromium.org/778703004/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerClient.idl:15: readonly attribute
DOMString frameType;
Do we want to ship those new attributes immediately or do we want to have them
ship with Push? (ie. .focus() is behind the PushMessaging flag)

[Kunihiko Sakamoto, 2014-12-09 02:30:35]

https://codereview.chromium.org/778703004/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerClient.idl (right):

https://codereview.chromium.org/778703004/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerClient.idl:15: readonly attribute
DOMString frameType;
On 2014/12/08 10:41:19, Mounir Lamouri wrote:
> Do we want to ship those new attributes immediately or do we want to have them
> ship with Push? (ie. .focus() is behind the PushMessaging flag)

These attributes can have non-Push use cases, so there's no reason to make them
blocked by Push.

There are some ongoing spec discussions though. If any spec issue arises, then
we can add a flag.

[mlamouri (slow - plz ping), 2014-12-09 09:44:15]

On 2014/12/09 02:30:35, Kunihiko Sakamoto wrote:
>
https://codereview.chromium.org/778703004/diff/80001/Source/modules/servicewo...
> File Source/modules/serviceworkers/ServiceWorkerClient.idl (right):
> 
>
https://codereview.chromium.org/778703004/diff/80001/Source/modules/servicewo...
> Source/modules/serviceworkers/ServiceWorkerClient.idl:15: readonly attribute
> DOMString frameType;
> On 2014/12/08 10:41:19, Mounir Lamouri wrote:
> > Do we want to ship those new attributes immediately or do we want to have
them
> > ship with Push? (ie. .focus() is behind the PushMessaging flag)
> 
> These attributes can have non-Push use cases, so there's no reason to make
them
> blocked by Push.
> 
> There are some ongoing spec discussions though. If any spec issue arises, then
> we can add a flag.

My understanding is that the spec isn't entirely ready yet. Are we comfortable
shipping that at the next release?

Maybe we could add a new flag for those?

[tkent, 2014-12-09 09:50:24]

On 2014/12/09 09:44:15, Mounir Lamouri wrote:
> > There are some ongoing spec discussions though. If any spec issue arises,
then
> > we can add a flag.
> 
> My understanding is that the spec isn't entirely ready yet. Are we comfortable
> shipping that at the next release?
> 
> Maybe we could add a new flag for those?

Yeah, it sounds we need to protect them by a flag.

[Kunihiko Sakamoto, 2014-12-09 10:40:01]

On 2014/12/09 09:50:24, tkent wrote:
> On 2014/12/09 09:44:15, Mounir Lamouri wrote:
> > > There are some ongoing spec discussions though. If any spec issue arises,
> then
> > > we can add a flag.
> > 
> > My understanding is that the spec isn't entirely ready yet. Are we
comfortable
> > shipping that at the next release?
> > 
> > Maybe we could add a new flag for those?
> 
> Yeah, it sounds we need to protect them by a flag.

OK, added a new flag ServiceWorkerClientAttributes.

[mlamouri (slow - plz ping), 2014-12-09 11:14:22]

lgtm, thanks! :)

[tkent, 2014-12-10 01:29:03]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-12-10 01:29:04]

lgtm

[commit-bot: I haz the power, 2014-12-10 01:29:49]

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

[Kunihiko Sakamoto, 2014-12-10 01:36:41]

The CQ bit was unchecked by ksakamoto@chromium.org

[Kunihiko Sakamoto, 2014-12-11 03:45:57]

The CQ bit was checked by ksakamoto@chromium.org

[commit-bot: I haz the power, 2014-12-11 03:47:18]

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

[commit-bot: I haz the power, 2014-12-11 07:56:31]

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