Service Worker: Add FetchEvent.clientId

Service Worker: Add FetchEvent.clientId

As per the latest spec update, this CL implements FetchEvent.clientId attribute.

Spec discussion: https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-clientid

Intent to implement and ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XlBHOIOCjUk

The layout tests for this CL are mostly taken from Mozilla's work instead of making new ones to ensure compatibility: https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552309
Committed: https://crrev.com/f1b2374078adf2068473bd9f81fd7615cf35d34d
Cr-Commit-Position: refs/heads/master@{#369649}

[jungkees, 2015-11-10 12:06:39]

Description was changed from

==========
(WIP) Service Worker: Add FetchEvent.cliendId

As per the latest spec update, this CL implements FetchEvent.clientId attribute.

It's WIP. Layout tests should be updated and added.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-...

BUG=552309
==========

to

==========
(WIP) Service Worker: Add FetchEvent.cliendId

As per the latest spec update, this CL implements FetchEvent.clientId attribute.

It's WIP. Layout tests should be updated and added.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-...

BUG=552309
==========

[jungkees, 2015-11-12 11:13:57]

The client concept needs to be clarified in the spec (SW, Fetch and HTML)
especially for navigation and some special cases such as sendBeacon(), ping="",
etc.

So far the spec contributors wanted FetchEvent.clientId returning undefined for
navigation.

[jungkees, 2015-12-23 15:09:12]

Patchset #2 (id:20001) has been deleted

[jungkees, 2015-12-24 04:19:25]

Patchset #2 (id:40001) has been deleted

[jungkees, 2015-12-24 09:03:29]

Description was changed from

==========
(WIP) Service Worker: Add FetchEvent.cliendId

As per the latest spec update, this CL implements FetchEvent.clientId attribute.

It's WIP. Layout tests should be updated and added.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-...

BUG=552309
==========

to

==========
Service Worker: Add FetchEvent.cliendId

As per the latest spec update, this CL implements FetchEvent.clientId attribute.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-...

BUG=552309
==========

[jungkees, 2015-12-24 09:03:29]

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

[jungkees, 2015-12-24 09:10:10]

I just updated a new snapshot ready for review (with a TODO marked in
fetch-event-client-id* to add a test case for a subresource request).

Re the value of .clientId for a navigation (non-subresource) request, I updated
both the spec
(https://github.com/slightlyoff/ServiceWorker/commit/872e51b9f06d9872d925a2877...)
and the implementation to set it to null.

PTAL when available. Merry Christmas! :)

[jungkees, 2015-12-29 07:54:59]

> I just updated a new snapshot ready for review (with a TODO marked in
> fetch-event-client-id* to add a test case for a subresource request).
> 

I added the test case for a subresource request. PTAL.

[falken, 2016-01-05 04:59:58]

Thanks for working on this! I have a compat concern about non-subresource
requests. Opened https://github.com/slightlyoff/ServiceWorker/issues/808

[jungkees, 2016-01-05 06:28:04]

On 2016/01/05 04:59:58, falken wrote:
> Thanks for working on this! I have a compat concern about non-subresource
> requests. Opened https://github.com/slightlyoff/ServiceWorker/issues/808

Left a comment in the spec issue. I'll address the result of the discussion.

[jungkees, 2016-01-06 10:28:47]

I made it conform to what Gecko does, but still want to have more comments on
the spec side. Please defer the review until the spec issue is resolved. I'll
update it.

[jungkees, 2016-01-09 03:32:19]

We finally decided to return null for a non-subresource requests:
https://github.com/slightlyoff/ServiceWorker/issues/808. I uploaded a new
snapshot having addressed that resolution. PTAL

[jungkees, 2016-01-09 03:37:00]

I'll send an intent to implement and ship on Monday. As discussed with falken@,
I'll make a single intent for FetchEvent.clientId and Clients.get(id). But
Mozilla's layout test for Clients.get(id) uses FetchEvent.clientId. So, I think
Clients.get(id) patch should be committed after FetchEvent.clientId patch lands.
Will it be okay?

[jungkees, 2016-01-13 02:06:55]

The intent to implement and ship has got three l-g-t-ms:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XlBHOIOCjUk.
Could you take a look?

[falken, 2016-01-13 09:06:53]

Looking good. CL description nits:
- cliendId -> clientId
- Indicate that the test changes are mostly taken from Mozilla, linking perhaps
to https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-event.html
(right):

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-event.html:84:
'Service Worker should respond to navigation fetch with null');
"no client id"

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-event.html:93:
'Service Worker should respond to fetch with an existing client id');
"with a client id"

Then we should do another fetch and ensure the client id is the same.

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-event.html:98: },
'Service Worker responds to fetch event with an existing client id');
"a client id"

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js
(right):

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js:102:
'006e6aae-cfd4-4331-bea8-fbae364703cf',
Curious: Firefox uses "test-client-id" here. Is that something our
implementation would fail on because it expects a UUID looking thing? Does the
spec say anything about the string format?

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebServiceWorkerRequest.cpp
(right):

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebServiceWorkerRequest.cpp:33: bool
m_isMainResourceLoad;
uninitialised bool

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRequest.h
(right):

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRequest.h:82:
void setClientId(WebString);
Can this be const WebString&?

[jungkees, 2016-01-13 10:54:51]

Description was changed from

==========
Service Worker: Add FetchEvent.cliendId

As per the latest spec update, this CL implements FetchEvent.clientId attribute.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-...

BUG=552309
==========

to

==========
Service Worker: Add FetchEvent.clientId

As per the latest spec update, this CL implements FetchEvent.clientId attribute.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-...

The layout tests for this CL are mostly taken from Mozilla's work instead of
making new ones to ensure compatibility:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552309
==========

[jungkees, 2016-01-13 10:55:18]

I just uploaded a new snapshot having addressed the comments. PTAL

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-event.html
(right):

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-event.html:84:
'Service Worker should respond to navigation fetch with null');
On 2016/01/13 09:06:53, falken wrote:
> "no client id"

Done.

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-event.html:93:
'Service Worker should respond to fetch with an existing client id');
On 2016/01/13 09:06:53, falken wrote:
> "with a client id"
> 
> Then we should do another fetch and ensure the client id is the same.

Done.

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-event.html:98: },
'Service Worker responds to fetch event with an existing client id');
On 2016/01/13 09:06:53, falken wrote:
> "a client id"

Done.

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js
(right):

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js:102:
'006e6aae-cfd4-4331-bea8-fbae364703cf',
On 2016/01/13 09:06:53, falken wrote:
> Curious: Firefox uses "test-client-id" here. Is that something our
> implementation would fail on because it expects a UUID looking thing?

Not actually. 'test-client-id' in this interface test will pass. But Chrome is
using a UUID for the client ids generated from the platform.

> Does the
> spec say anything about the string format?

It used to specify it as a UUID. But in the past f2f, we changed it to just an
opaque string not mandating a UUID.

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebServiceWorkerRequest.cpp
(right):

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebServiceWorkerRequest.cpp:33: bool
m_isMainResourceLoad;
Changed it set to false in the initializer list.

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRequest.h
(right):

https://codereview.chromium.org/1435633002/diff/130001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerRequest.h:82:
void setClientId(WebString);
On 2016/01/13 09:06:53, falken wrote:
> Can this be const WebString&?

Done.

[falken, 2016-01-14 02:30:15]

LGTM!

https://codereview.chromium.org/1435633002/diff/150001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job.h (right):

https://codereview.chromium.org/1435633002/diff/150001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.h:231: std::string
client_id_;
nit: I like to order things in the same order as the ctor args if possible. This
probably belongs just before blob_storage_context_.

[jungkees, 2016-01-14 04:54:29]

Description was changed from

==========
Service Worker: Add FetchEvent.clientId

As per the latest spec update, this CL implements FetchEvent.clientId attribute.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-...

The layout tests for this CL are mostly taken from Mozilla's work instead of
making new ones to ensure compatibility:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552309
==========

to

==========
Service Worker: Add FetchEvent.clientId

As per the latest spec update, this CL implements FetchEvent.clientId attribute.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-...

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XlBHOIOCjUk

The layout tests for this CL are mostly taken from Mozilla's work instead of
making new ones to ensure compatibility:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552309
==========

[jungkees, 2016-01-14 04:54:29]

jungkee.song@samsung.com changed reviewers:
+ dcheng@chromium.org, palmer@chromium.org

[jungkees, 2016-01-14 04:56:46]

falken@, done with the comment.

tkent@, could you review the public APIs?

dcheng@, palmer@, could you review the IPC messages?

https://codereview.chromium.org/1435633002/diff/150001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job.h (right):

https://codereview.chromium.org/1435633002/diff/150001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.h:231: std::string
client_id_;
On 2016/01/14 02:30:15, falken wrote:
> nit: I like to order things in the same order as the ctor args if possible.
This
> probably belongs just before blob_storage_context_.

Done.

[tkent, 2016-01-14 05:03:42]

Web-exposed API change LGTM

[dcheng, 2016-01-14 19:10:53]

ipc changes lgtm

[jungkees, 2016-01-15 00:15:55]

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

[jungkees, 2016-01-15 00:15:55]

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/1435633002/#ps170001
(title: "Change the order of a class member")

[commit-bot: I haz the power, 2016-01-15 00:17:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1435633002/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1435633002/170001

[commit-bot: I haz the power, 2016-01-15 01:41:36]

Description was changed from

==========
Service Worker: Add FetchEvent.clientId

As per the latest spec update, this CL implements FetchEvent.clientId attribute.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-...

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XlBHOIOCjUk

The layout tests for this CL are mostly taken from Mozilla's work instead of
making new ones to ensure compatibility:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552309
==========

to

==========
Service Worker: Add FetchEvent.clientId

As per the latest spec update, this CL implements FetchEvent.clientId attribute.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-...

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XlBHOIOCjUk

The layout tests for this CL are mostly taken from Mozilla's work instead of
making new ones to ensure compatibility:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552309
==========

[commit-bot: I haz the power, 2016-01-15 01:41:37]

Committed patchset #8 (id:170001)

[commit-bot: I haz the power, 2016-01-15 01:42:56]

Description was changed from

==========
Service Worker: Add FetchEvent.clientId

As per the latest spec update, this CL implements FetchEvent.clientId attribute.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-...

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XlBHOIOCjUk

The layout tests for this CL are mostly taken from Mozilla's work instead of
making new ones to ensure compatibility:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552309
==========

to

==========
Service Worker: Add FetchEvent.clientId

As per the latest spec update, this CL implements FetchEvent.clientId attribute.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-...

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XlBHOIOCjUk

The layout tests for this CL are mostly taken from Mozilla's work instead of
making new ones to ensure compatibility:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552309

Committed: https://crrev.com/f1b2374078adf2068473bd9f81fd7615cf35d34d
Cr-Commit-Position: refs/heads/master@{#369649}
==========

[commit-bot: I haz the power, 2016-01-15 01:42:57]

Patchset 8 (id:??) landed as
https://crrev.com/f1b2374078adf2068473bd9f81fd7615cf35d34d
Cr-Commit-Position: refs/heads/master@{#369649}

[huangs, 2016-01-15 15:15:42]

A revert of this CL (patchset #8 id:170001) has been created in
https://codereview.chromium.org/1588003005/ by huangs@chromium.org.

The reason for reverting is: This CL is suspected to break Webkit Linux ASAN
tests:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b...

==4==WARNING: MemorySanitizer: use-of-uninitialized-value
...
#7 0xe5d7fd5 in Write
content/common/service_worker/service_worker_messages.h:55:0

Please check initializaiton paths, and run on more ASAN bots before relanding..

[jungkees, 2016-01-19 03:40:18]

This change relanded with a patch in https://codereview.chromium.org/1597383002/