Service Worker: Surface Client.id attribute.

Service Worker: Surface Client.id attribute.

Implement Client.id method as per spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#client-id-attribute

Spec issue: https://github.com/slightlyoff/ServiceWorker/issues/634
More context: https://github.com/slightlyoff/ServiceWorker/issues/609

Previous work by kinuko@
Blink-side patch: https://codereview.chromium.org/990583002/
Chromium-side patch: https://codereview.chromium.org/988063004/

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

[jungkees, 2015-06-24 07:17:27]

jungkee.song@samsung.com changed reviewers:
+ l.gombos@samsung.com

[jungkees, 2015-06-24 07:19:57]

This CL exposes service worker client's id to script surface. PTAL

[lgombos, 2015-06-25 01:04:22]

peer review looks good to me

https://codereview.chromium.org/1209493002/diff/1/LayoutTests/http/tests/serv...
File LayoutTests/http/tests/serviceworker/client-id.html (right):

https://codereview.chromium.org/1209493002/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/client-id.html:33: var pattern = new RegExp
(['^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0',
Not sure if this regexp is general enough. Does this allow capital letters ?

https://codereview.chromium.org/1209493002/diff/1/Source/modules/serviceworke...
File Source/modules/serviceworkers/Client.idl (right):

https://codereview.chromium.org/1209493002/diff/1/Source/modules/serviceworke...
Source/modules/serviceworkers/Client.idl:15:
[RuntimeEnabled=ServiceWorkerClientAttributes] readonly attribute DOMString id;
This is a webexposed change. Do you need an intent to ship discussion on
blink-dev for htis just like this one -
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64 .

[lgombos, 2015-06-25 01:10:35]

https://codereview.chromium.org/1209493002/diff/1/LayoutTests/http/tests/serv...
File LayoutTests/http/tests/serviceworker/client-id.html (right):

https://codereview.chromium.org/1209493002/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/client-id.html:33: var pattern = new RegExp
(['^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0',
On 2015/06/25 01:04:21, lgombos wrote:
> Not sure if this regexp is general enough. Does this allow capital letters ? 

Sorry I missed the 'i' at the end, but why [1-5] instead of [0-9a-f] ?

[jungkees, 2015-06-25 01:51:36]

Thanks for peer review!

https://codereview.chromium.org/1209493002/diff/1/LayoutTests/http/tests/serv...
File LayoutTests/http/tests/serviceworker/client-id.html (right):

https://codereview.chromium.org/1209493002/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/client-id.html:33: var pattern = new RegExp
(['^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0',
According to RFC4122, that byte can hold the value 1 to 5 which are valid UUID
version. (See https://www.ietf.org/rfc/rfc4122.txt section 4.1.3). FYI, [89ab]
comes from the protocol from clock_seq_hi_and_reserved bits. (See 4.2.2 in the
same spec). Thanks.

https://codereview.chromium.org/1209493002/diff/1/Source/modules/serviceworke...
File Source/modules/serviceworkers/Client.idl (right):

https://codereview.chromium.org/1209493002/diff/1/Source/modules/serviceworke...
Source/modules/serviceworkers/Client.idl:15:
[RuntimeEnabled=ServiceWorkerClientAttributes] readonly attribute DOMString id;
I'm not quite sure about the requirement. This is part of the interface already
shipped, but I'll have to double check. Will discuss with the service worker
owners. Thanks for review.

[jungkees, 2015-06-25 01:53:03]

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

[jungkees, 2015-06-25 01:55:26]

PTAL Also could you guide me whether it needs intent to ship or some other
procedure?

[kinuko, 2015-06-25 02:23:04]

Looking good to me, I defer the decision if we need intent-to-ship for this to
falken@.

https://codereview.chromium.org/1209493002/diff/1/LayoutTests/http/tests/serv...
File LayoutTests/http/tests/serviceworker/client-id.html (right):

https://codereview.chromium.org/1209493002/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/client-id.html:32: // A regex object for
UUID validation
nit: end comments with period. Also it might be nicer to include the RFC link?

[jungkees, 2015-06-25 02:55:45]

Addressed the comment. Thanks for review.

[falken, 2015-06-25 04:09:57]

Since I don't think this has any risk of compat issues, I consider this a
trivial change and won't require an Intent to Ship. However, lately OWNERs want
OWP bugs and status dashboard entries, so I'll add those.

https://codereview.chromium.org/1209493002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/client-id.html (right):

https://codereview.chromium.org/1209493002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/client-id.html:8: var t = async_test('Test
Client.id');
our style guide does async_test(function(t) { //test }, 'description');
https://www.chromium.org/blink/serviceworker/testing

https://codereview.chromium.org/1209493002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/client-id.html:19: f.focus();
why do we need to focus()?

https://codereview.chromium.org/1209493002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/client-id.html:33: var pattern = new RegExp
(['^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0',
nit: no space before the (

https://codereview.chromium.org/1209493002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/client-id.html:35: function
validateUUID(id) {
nit: validate_uuid

https://codereview.chromium.org/1209493002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/client-id.html:39: function onMessage(e) {
nit: on_message

https://codereview.chromium.org/1209493002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/client-id-worker.js (right):

https://codereview.chromium.org/1209493002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/client-id-worker.js:12:
.then(function() {
why is this a new function, it looks like the previous function doesn't return a
promise?

[jungkees, 2015-06-25 07:48:37]

Uploaded a new snapshot having addressed the comments. PTAL

https://codereview.chromium.org/1209493002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/client-id.html (right):

https://codereview.chromium.org/1209493002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/client-id.html:8: var t = async_test('Test
Client.id');
Right. Corrected it accordingly.

https://codereview.chromium.org/1209493002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/client-id.html:19: f.focus();
This is to be sure Clients.matchAll() calls iterate in the same order. (it
iterates in most recently focused order.) Added a comment.

https://codereview.chromium.org/1209493002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/client-id-worker.js (right):

https://codereview.chromium.org/1209493002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/client-id-worker.js:12:
.then(function() {
The first forEach implicitly returns undefined, but it still guarantees the
second matchAll() is called after the client ids for the first matchAll() are
pushed in the result array. But I updated the code so it reads more clear in
that aspect.

[falken, 2015-06-26 02:21:51]

lgtm can you change the BUG= line to point to 504222

https://codereview.chromium.org/1209493002/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/client-id.html (right):

https://codereview.chromium.org/1209493002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/client-id.html:40: return id.match(pattern)
? true : false;
This looks weird to me but I may be unfamiliar with intricacies of JS
truthiness. Can it just be "return id.match(pattern);" or "return
!!id.match(pattern);"

[jungkees, 2015-06-26 02:43:23]

Addressed the comment. Thanks.

https://codereview.chromium.org/1209493002/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/client-id.html (right):

https://codereview.chromium.org/1209493002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/client-id.html:40: return id.match(pattern)
? true : false;
Yes "return !!id.match(pattern);" makes sense for my usage here. match() itself
returns null when the pattern doesn't match. Addressed it so.

[jungkees, 2015-06-26 02:43:35]

The CQ bit was checked by jungkee.song@samsung.com

[jungkees, 2015-06-26 02:43:36]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/1209493002/#ps60001
(title: "Update JS expression for truthiness.")

[commit-bot: I haz the power, 2015-06-26 02:43:56]

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

[commit-bot: I haz the power, 2015-06-26 03:41:02]

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