Implement support for link type serviceworker in link elements.

Implement support for link type serviceworker in link elements.

After a previous CL landed support for link rel=serviceworker in Link HTTP
headers, this CL adds similar support to link elements as specified in
https://github.com/slightlyoff/ServiceWorker/pull/828

BUG=582310
Committed: https://crrev.com/ac06ca5985e51a21ac2d8711eb9671eb55452f95
Cr-Commit-Position: refs/heads/master@{#382733}

[Marijn Kruisselbrink, 2016-03-09 23:49:16]

Patchset #3 (id:40001) has been deleted

[Marijn Kruisselbrink, 2016-03-16 22:52:34]

Description was changed from

==========
WIP link type serviceworker in link elements

BUG=
==========

to

==========
Implement support for link type serviceworker in link elements.

After a previous CL landed support for link rel=serviceworker in Link HTTP
headers, this CL adds similar support to link elements as specified in
https://github.com/slightlyoff/ServiceWorker/pull/828

BUG=582310
==========

[Marijn Kruisselbrink, 2016-03-16 23:05:46]

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

[Marijn Kruisselbrink, 2016-03-16 23:05:47]

Probably most of this is more something for a core/ OWNER, but no idea who would
be a sensible one as HTMLLinkElement doesn't really seem to have any particular
set of people who review changes to it, and seems to be quite a mess as a
result...

Anyway, I'm not convinced that the way I'm hooking into HTMLLinkElement is the
correct way (although it works fine). Also I'm not convinced that using
FrameLoaderClient to get calls from core/ piped through via web/ to
modules/serviceworkers/ is the best way to do this, as this isn't really related
to loading frames. But adding a new abstraction somewhere didn't seem worth it
either, and I could find any better way to do this either. So suggestions are
welcome.

[falken, 2016-03-17 03:57:24]

lgtm but the win_chromium_rel_ng trybot error looks real.

I agree using FrameLoaderClient to pipe from core/ to modules/ may be odd, but
have no better suggestion. FrameLoaderClient already has SW-related functions
anyway. Let's see what core, loading, and web owners say.

https://codereview.chromium.org/1781783002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/LinkServiceWorker.cpp (right):

https://codereview.chromium.org/1781783002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/LinkServiceWorker.cpp:1: // Copyright 2014
The Chromium Authors. All rights reserved.
2016

[Yoav Weiss, 2016-03-17 11:21:13]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2016-03-17 11:21:14]

core/ LGTM

https://codereview.chromium.org/1781783002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLLinkElement.cpp (left):

https://codereview.chromium.org/1781783002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:233: } else {
Unrelated to this CL but we really need to clean this up and get other links'
processing out of LinkStyle::process

[Marijn Kruisselbrink, 2016-03-17 19:53:34]

On 2016/03/17 at 03:57:24, falken wrote:
> the win_chromium_rel_ng trybot error looks real.

Ah yes, that's indeed real test flakyness. Fixed by checking the script url of
the "newest" worker rather than assuming that the newest worker is still always
"installing" when the register promise resolves (for the javascript
serviceWorker.register() case it is true that the installing worker is always
still at "installing" when its promise resolves, but even though the link
element fires its onloaded event at the same time, actually getting a
ServiceWorkerRegistration requires another async API call, which introduces a
race between the installing worker becoming waiting and my "polyfill register on
top of the link element" method resolving its promise).

https://codereview.chromium.org/1781783002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLLinkElement.cpp (left):

https://codereview.chromium.org/1781783002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:233: } else {
On 2016/03/17 at 11:21:13, Yoav Weiss wrote:
> Unrelated to this CL but we really need to clean this up and get other links'
processing out of LinkStyle::process

Agreed, although it's not trivial to do so and stay web compatible (technically
having different LinkResource implementations for different rel values already
isn't entirely spec compliant; it's perfectly legal according to the spec to
have a link element with <link rel="manifest serviceworker stylesheet">. That
would of course be pretty weird, and likely whatever href is specified for the
link will only load correctly for one of the different link types, but a browser
can't know which one it is without trying all of them).

So not sure what a cleaned up/redesigned HTMLLinkElement/LinkResource codebase
would look like, but it would be nice to somehow figure that out/clean this up.

https://codereview.chromium.org/1781783002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/LinkServiceWorker.cpp (right):

https://codereview.chromium.org/1781783002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/LinkServiceWorker.cpp:1: // Copyright 2014
The Chromium Authors. All rights reserved.
On 2016/03/17 at 03:57:23, falken wrote:
> 2016

Fixed

[Marijn Kruisselbrink, 2016-03-17 21:17:29]

mek@chromium.org changed reviewers:
+ japhet@chromium.org

[Marijn Kruisselbrink, 2016-03-17 21:17:30]

+japhet for Source/web/ OWNERS

[Nate Chapin, 2016-03-17 22:49:04]

https://codereview.chromium.org/1781783002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1781783002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1079:
scopeURL.removeFragmentIdentifier();
Can we do this and the things about it in LinkServiceWorker? Ideally
FrameLoaderClientImpl will be plumbing-only.

[Nate Chapin, 2016-03-17 22:49:16]

On 2016/03/17 22:49:04, Nate Chapin wrote:
>
https://codereview.chromium.org/1781783002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/1781783002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1079:
> scopeURL.removeFragmentIdentifier();
> Can we do this and the things about it in LinkServiceWorker? Ideally
> FrameLoaderClientImpl will be plumbing-only.

s/about/above/

[Marijn Kruisselbrink, 2016-03-17 23:20:39]

https://codereview.chromium.org/1781783002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1781783002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1079:
scopeURL.removeFragmentIdentifier();
On 2016/03/17 at 22:49:03, Nate Chapin wrote:
> Can we do this and the things above it in LinkServiceWorker? Ideally
FrameLoaderClientImpl will be plumbing-only.

Done

[falken, 2016-03-18 00:29:53]

Note: the red "linux_blink_oilpan_rel" bot (which is actually non-oilpan) this
time is an existing issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=594230

[falken, 2016-03-18 01:11:43]

still lgtm with comment

https://codereview.chromium.org/1781783002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/registration-tests.js
(right):

https://codereview.chromium.org/1781783002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/registration-tests.js:2:
function get_newest_worker(registration) {
Many of our tests use registration.installing in the then() for register, which
I think is guaranteed by the spec (otherwise, we should convert them all to
get_newest_worker). Can you add a comment similar to your codereview comment
about why get_newest_worker is needed?

[Marijn Kruisselbrink, 2016-03-18 21:20:06]

Thinking more about the core vs. web vs. modules split I wonder if maybe the
code would be cleaner if I move LinkServiceWorker into modules/serviceworker.
web/FrameLoaderClientImpl would then only be responsible for creating a new
LinkServiceWorker instance, and the code in core/ would also not need to know
quite as much about service workers...

https://codereview.chromium.org/1781783002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/registration-tests.js
(right):

https://codereview.chromium.org/1781783002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/registration-tests.js:2:
function get_newest_worker(registration) {
On 2016/03/18 at 01:11:43, falken wrote:
> Many of our tests use registration.installing in the then() for register,
which I think is guaranteed by the spec (otherwise, we should convert them all
to get_newest_worker). Can you add a comment similar to your codereview comment
about why get_newest_worker is needed?

Done

[Marijn Kruisselbrink, 2016-03-18 22:05:29]

Okay, in the latest patch I moved the serviceworker LinkResource implementation
to modules/serviceworkers and reduced the code in web/FrameLoaderClientImpl to
only create such an instance. I think this is a cleaner separation of concerns
then what I had before.

[Nate Chapin, 2016-03-18 22:12:03]

web/ and core/ LGTM

https://codereview.chromium.org/1781783002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/1781783002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoaderClient.h:61: class
LinkLoaderClient;
Is this needed?

[Marijn Kruisselbrink, 2016-03-18 22:18:37]

https://codereview.chromium.org/1781783002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/1781783002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoaderClient.h:61: class
LinkLoaderClient;
On 2016/03/18 at 22:12:03, Nate Chapin wrote:
> Is this needed?

No, it isn't. Removed.

[Marijn Kruisselbrink, 2016-03-18 23:30:17]

mek@chromium.org changed reviewers:
+ leviw@chromium.org

[Marijn Kruisselbrink, 2016-03-18 23:30:18]

+leviw for RuntimeEnabledFeatures.in OWNERS

[Marijn Kruisselbrink, 2016-03-22 17:39:00]

mek@chromium.org changed reviewers:
+ kbr@chromium.org
- leviw@chromium.org

[Marijn Kruisselbrink, 2016-03-22 17:39:01]

+kbr for RuntimeEnabledFeatures OWNERS

[jbroman, 2016-03-22 22:09:12]

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

[jbroman, 2016-03-22 22:09:13]

platform/RuntimeEnabledFeatures.in lgtm

https://codereview.chromium.org/1781783002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1781783002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:112:
LinkServiceworker status=experimental
nit: why no uppercase W for Worker?

[Marijn Kruisselbrink, 2016-03-22 22:23:30]

https://codereview.chromium.org/1781783002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1781783002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:112:
LinkServiceworker status=experimental
On 2016/03/22 at 22:09:13, jbroman wrote:
> nit: why no uppercase W for Worker?

agreed, uppercase W is probably nicer/more consistent.

[Marijn Kruisselbrink, 2016-03-22 22:24:15]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-03-22 22:24:16]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws,
falken@chromium.org, japhet@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1781783002/#ps240001
(title: "uppercase W in runtime feature name")

[commit-bot: I haz the power, 2016-03-22 22:24:39]

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

[Ken Russell (switch to Gerrit), 2016-03-22 23:24:10]

RuntimeEnabledFeatures.in lgtm after the fact.

[commit-bot: I haz the power, 2016-03-22 23:44:08]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2016-03-22 23:45:38]

Description was changed from

==========
Implement support for link type serviceworker in link elements.

After a previous CL landed support for link rel=serviceworker in Link HTTP
headers, this CL adds similar support to link elements as specified in
https://github.com/slightlyoff/ServiceWorker/pull/828

BUG=582310
==========

to

==========
Implement support for link type serviceworker in link elements.

After a previous CL landed support for link rel=serviceworker in Link HTTP
headers, this CL adds similar support to link elements as specified in
https://github.com/slightlyoff/ServiceWorker/pull/828

BUG=582310

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

[commit-bot: I haz the power, 2016-03-22 23:45:39]

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