Start of foreign fetch implementation.

Start of foreign fetch implementation.

This changes the install event to use a new InstallEvent interface,
with a method to register foreign fetch scopes. Calling that method
will end up storing the scopes in the SWVersion in the browser process.

BUG=540509
Committed: https://crrev.com/26507d0e00b7beb248d751512cc045c9bee3e359
Cr-Commit-Position: refs/heads/master@{#354845}

[Marijn Kruisselbrink, 2015-10-14 22:43:55]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2015-10-15 00:28:32]

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

[bsittler, 2015-10-15 19:38:00]

Why is the switch to the new event interface contingent on foreign fetch being
enabled?

https://codereview.chromium.org/1406823002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1406823002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1880: DVLOG(1) <<
"Received unexpected invalid URL from renderer process.";
Should there be test coverage for this defense-in-depth?

[Marijn Kruisselbrink, 2015-10-15 22:19:04]

On 2015/10/15 at 19:38:00, bsittler wrote:
> Why is the switch to the new event interface contingent on foreign fetch being
enabled?
Because I don't want to switch a new event interface before it's gone through an
intent-to-ship. So since for now foreign fetch is the only reason the
InstallEvent interface exists, using it is behind the FF feature flag.

https://codereview.chromium.org/1406823002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1406823002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1880: DVLOG(1) <<
"Received unexpected invalid URL from renderer process.";
On 2015/10/15 at 19:37:59, bsittler wrote:
> Should there be test coverage for this defense-in-depth?

Good point, I added some.

[bsittler, 2015-10-15 22:59:46]

lgtm

[Marijn Kruisselbrink, 2015-10-15 23:00:45]

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

[Marijn Kruisselbrink, 2015-10-15 23:00:46]

+falken for service worker OWNERS

[falken, 2015-10-16 11:58:44]

This looks good, just style nits.

https://codereview.chromium.org/1406823002/diff/40001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/1406823002/diff/40001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:73: wrapper_(new
ServiceWorkerContextWrapper(NULL)),
nit: nullptr for new code (and below)

https://codereview.chromium.org/1406823002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1406823002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1872: // Renderer
should have already verified all these urls are inside the workers
nit: worker's

https://codereview.chromium.org/1406823002/diff/40001/content/common/service_...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/1406823002/diff/40001/content/common/service_...
content/common/service_worker/service_worker_messages.h:294: // handle. Should
only be send while an install event is being handled.
nit: sent

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/install-event-type-worker.js
(right):

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/install-event-type-worker.js:5:
assert_true(event instanceof InstallEvent);
Hm, I was a bit worried this expectation changes even though foreign fetch is in
experimental, but I guess it's not possible to ship InstallEvent without the
global-interface-listing-service-worker failing in the webexposed suite, so
we're not losing test coverage. So it's OK.

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js
(right):

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js:1:

This is in http/tests/serviceworker so should use service worker's test style:
https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style

Notably it uses two-space indent and function arguments are written like:
self.addEventListener('install, function(event) {
    var scope = ...;
  });

By the way lately I'm using ES6 arrow functions, which you might also prefer.

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js:4:
var scopeUrl = new URL(scope);
nit: scope_url (and snake_case throughout this file)

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js:20:
event.registerForeignFetchScopes(["/foo"]);
nit: single-quotes for javascript string literals (and throughout this file)

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js:66:
// runs first.
Curious: why does that matter?

[Marijn Kruisselbrink, 2015-10-16 17:23:25]

https://codereview.chromium.org/1406823002/diff/40001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/1406823002/diff/40001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:73: wrapper_(new
ServiceWorkerContextWrapper(NULL)),
On 2015/10/16 at 11:58:43, falken wrote:
> nit: nullptr for new code (and below)

Done

https://codereview.chromium.org/1406823002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1406823002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1872: // Renderer
should have already verified all these urls are inside the workers
On 2015/10/16 at 11:58:43, falken wrote:
> nit: worker's

Done

https://codereview.chromium.org/1406823002/diff/40001/content/common/service_...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/1406823002/diff/40001/content/common/service_...
content/common/service_worker/service_worker_messages.h:294: // handle. Should
only be send while an install event is being handled.
On 2015/10/16 at 11:58:43, falken wrote:
> nit: sent

Done

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/install-event-type-worker.js
(right):

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/install-event-type-worker.js:5:
assert_true(event instanceof InstallEvent);
On 2015/10/16 at 11:58:43, falken wrote:
> Hm, I was a bit worried this expectation changes even though foreign fetch is
in experimental, but I guess it's not possible to ship InstallEvent without the
global-interface-listing-service-worker failing in the webexposed suite, so
we're not losing test coverage. So it's OK.

Yeah, the version of global-interface-listing-service-worker test that runs
without the experimental web platform features enabled should be enough to make
sure InstallEvent isn't shipped to stable.

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js
(right):

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js:1:

On 2015/10/16 at 11:58:43, falken wrote:
> This is in http/tests/serviceworker so should use service worker's test style:
> https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style
> 
> Notably it uses two-space indent and function arguments are written like:
> self.addEventListener('install, function(event) {
>     var scope = ...;
>   });

Done.

> By the way lately I'm using ES6 arrow functions, which you might also prefer.

I do like arrow functions, although I don't think there is really any benefit to
them in this case.

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js:4:
var scopeUrl = new URL(scope);
On 2015/10/16 at 11:58:43, falken wrote:
> nit: scope_url (and snake_case throughout this file)

Done

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js:20:
event.registerForeignFetchScopes(["/foo"]);
On 2015/10/16 at 11:58:43, falken wrote:
> nit: single-quotes for javascript string literals (and throughout this file)

Done

https://codereview.chromium.org/1406823002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js:66:
// runs first.
On 2015/10/16 at 11:58:43, falken wrote:
> Curious: why does that matter?

It matters because the install handler in testharness sets the "all tests have
been added" flag, so as soon as the first test finishes after the install
handler has run testharness will think it is done.

[Marijn Kruisselbrink, 2015-10-16 17:37:40]

mek@chromium.org changed reviewers:
+ tkent@chromium.org, tsepez@chromium.org

[Marijn Kruisselbrink, 2015-10-16 17:37:41]

+tkent for WebKit/Source/web and WebKit/Source/platform OWNERS
+tsepez for IPC OWNERS

[Tom Sepez, 2015-10-16 18:12:39]

https://codereview.chromium.org/1406823002/diff/60001/content/common/service_...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/1406823002/diff/60001/content/common/service_...
content/common/service_worker/service_worker_messages.h:294: // handle. Should
only be sent while an install event is being handled.
What happens if an evil renderer doesn't adhere to this restriction?

[Marijn Kruisselbrink, 2015-10-16 18:29:10]

https://codereview.chromium.org/1406823002/diff/60001/content/common/service_...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/1406823002/diff/60001/content/common/service_...
content/common/service_worker/service_worker_messages.h:294: // handle. Should
only be sent while an install event is being handled.
On 2015/10/16 at 18:12:39, Tom Sepez wrote:
> What happens if an evil renderer doesn't adhere to this restriction?

Currently the browser process DCHECKs this (the same way OnInstallEventFinished
and OnActivateEventFinished DCHECK if they are called when the service worker is
not in the correct state).
In case DCHECKs are enabled the browser just accepts the IPC anyway (and still
checks if the URLs are valid, within the scope of the worker, etc). If the
worker isn't installing though, any urls received will probably not be stored
(since storing generally happens on completion of the install event), so they'll
probably not end up being used. Foreign fetch hings might behave unpredictably,
but renderers won't be able to do anything this way a well behaved
renderer/service worker wouldn't be able to do anyway.

[Tom Sepez, 2015-10-16 18:32:11]

OK LGTM

[falken, 2015-10-19 03:29:23]

lgtm

https://codereview.chromium.org/1406823002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js
(right):

https://codereview.chromium.org/1406823002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js:1:

nit: extra line

[tkent, 2015-10-19 03:31:18]

WebKit/Source/web and WebKit/Source/platform lgtm

[Marijn Kruisselbrink, 2015-10-19 17:06:55]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2015-10-19 17:06:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
bsittler@chromium.org, tkent@chromium.org, falken@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1406823002/#ps80001
(title: "remove blank line")

[commit-bot: I haz the power, 2015-10-19 17:08:02]

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

[commit-bot: I haz the power, 2015-10-19 18:21:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-19 18:21:53]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Marijn Kruisselbrink, 2015-10-19 18:33:41]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2015-10-19 18:36:51]

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

[commit-bot: I haz the power, 2015-10-19 20:12:50]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-10-19 20:13:39]

Patchset 4 (id:??) landed as
https://crrev.com/26507d0e00b7beb248d751512cc045c9bee3e359
Cr-Commit-Position: refs/heads/master@{#354845}