ServiceWorker: Support non-window clients in Clients.matchAll (2/2 blink)

ServiceWorker: Support non-window clients in Clients.matchAll (2/2 blink)

- Supports ClientQueryOptions.type (Intent-to-ship thread: http://goo.gl/K5QZPr)
- Supports Client.frameType (behind flag)

Depends on corresponding chromium patch: https://codereview.chromium.org/1042933002/

BUG=460415
TEST=http/tests/serviceworker/clients-matchall-client-types.html

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

[falken, 2015-03-31 04:34:31]

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

[falken, 2015-03-31 04:34:32]

lgtm

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
File LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html
(right):

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html:9: var
worker_url = scope + '-worker.js';
nit: hmm we've been using -worker.js for service workers. Would it make sense to
name this -sharedworker.js or -shared-worker.js?
(I was momentarily confused reviewing clients-matchall-client-types-worker.js
thinking it was a service worker.)

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html:13:
['visible', true, new URL(iframe_url, location).toString(), 'nested']
nit: these .toString() could be .href to save typing

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html:36:
resolve(frame);
nit: this return value isn't used

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html:57:
resolve(e.data);
nit: unused

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html:58: }
nit: missing semi-colon

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html:66:
{type:"window"});
nit: should be single-quotes in JS (same below)

https://codereview.chromium.org/1040993003/diff/1/Source/modules/serviceworke...
File Source/modules/serviceworkers/ServiceWorkerClients.cpp (right):

https://codereview.chromium.org/1040993003/diff/1/Source/modules/serviceworke...
Source/modules/serviceworkers/ServiceWorkerClients.cpp:33:
WebServiceWorkerClientInfo& client = webClients->clients[i];
can this be const?

[kinuko, 2015-03-31 05:19:28]

Thx, sorry can someone take over these 2 patches?
2015/03/31 午後1:34 <falken@chromium.org>:

> lgtm
>
>
> https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/
> serviceworker/clients-matchall-client-types.html
> File
> LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html
> (right):
>
> https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/
> serviceworker/clients-matchall-client-types.html#newcode9
> LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html:9:
> var worker_url = scope + '-worker.js';
> nit: hmm we've been using -worker.js for service workers. Would it make
> sense to name this -sharedworker.js or -shared-worker.js?
> (I was momentarily confused reviewing
> clients-matchall-client-types-worker.js thinking it was a service
> worker.)
>
> https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/
> serviceworker/clients-matchall-client-types.html#newcode13
> LayoutTests/http/tests/serviceworker/clients-
> matchall-client-types.html:13:
> ['visible', true, new URL(iframe_url, location).toString(), 'nested']
> nit: these .toString() could be .href to save typing
>
> https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/
> serviceworker/clients-matchall-client-types.html#newcode36
> LayoutTests/http/tests/serviceworker/clients-
> matchall-client-types.html:36:
> resolve(frame);
> nit: this return value isn't used
>
> https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/
> serviceworker/clients-matchall-client-types.html#newcode57
> LayoutTests/http/tests/serviceworker/clients-
> matchall-client-types.html:57:
> resolve(e.data);
> nit: unused
>
> https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/
> serviceworker/clients-matchall-client-types.html#newcode58
> LayoutTests/http/tests/serviceworker/clients-
> matchall-client-types.html:58:
> }
> nit: missing semi-colon
>
> https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/
> serviceworker/clients-matchall-client-types.html#newcode66
> LayoutTests/http/tests/serviceworker/clients-
> matchall-client-types.html:66:
> {type:"window"});
> nit: should be single-quotes in JS (same below)
>
> https://codereview.chromium.org/1040993003/diff/1/Source/
> modules/serviceworkers/ServiceWorkerClients.cpp
> File Source/modules/serviceworkers/ServiceWorkerClients.cpp (right):
>
> https://codereview.chromium.org/1040993003/diff/1/Source/
> modules/serviceworkers/ServiceWorkerClients.cpp#newcode33
> Source/modules/serviceworkers/ServiceWorkerClients.cpp:33:
> WebServiceWorkerClientInfo& client = webClients->clients[i];
> can this be const?
>
> https://codereview.chromium.org/1040993003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[nhiroki, 2015-03-31 21:57:08]

FYI: I'll commit this on behalf of kinuko@ and will make a separate CL to clean
up as per falken@' comments (already confirmed it with falken@)

[nhiroki, 2015-03-31 21:57:21]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-03-31 21:58:59]

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

[commit-bot: I haz the power, 2015-03-31 22:09:36]

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

[nhiroki, 2015-04-01 04:51:34]

Uploaded a follow-up CL: https://codereview.chromium.org/1045363004/

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
File LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html
(right):

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html:9: var
worker_url = scope + '-worker.js';
On 2015/03/31 04:34:32, falken wrote:
> nit: hmm we've been using -worker.js for service workers. Would it make sense
to
> name this -sharedworker.js or -shared-worker.js?
> (I was momentarily confused reviewing clients-matchall-client-types-worker.js
> thinking it was a service worker.)

Renamed to -shared-worker.js

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html:13:
['visible', true, new URL(iframe_url, location).toString(), 'nested']
On 2015/03/31 04:34:32, falken wrote:
> nit: these .toString() could be .href to save typing

Done.

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html:36:
resolve(frame);
On 2015/03/31 04:34:32, falken wrote:
> nit: this return value isn't used

Removed.

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html:57:
resolve(e.data);
On 2015/03/31 04:34:32, falken wrote:
> nit: unused

Removed.

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html:58: }
On 2015/03/31 04:34:32, falken wrote:
> nit: missing semi-colon

Done.

https://codereview.chromium.org/1040993003/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/clients-matchall-client-types.html:66:
{type:"window"});
On 2015/03/31 04:34:31, falken wrote:
> nit: should be single-quotes in JS (same below)

Done.

https://codereview.chromium.org/1040993003/diff/1/Source/modules/serviceworke...
File Source/modules/serviceworkers/ServiceWorkerClients.cpp (right):

https://codereview.chromium.org/1040993003/diff/1/Source/modules/serviceworke...
Source/modules/serviceworkers/ServiceWorkerClients.cpp:33:
WebServiceWorkerClientInfo& client = webClients->clients[i];
On 2015/03/31 04:34:32, falken wrote:
> can this be const?

Done.