Add origins argument to registerForeignFetchScopes.

Add origins argument to registerForeignFetchScopes.

Adds the new argument, stores the list of origins for which to intercept
requests, and adds origin checks to the interception code. Also updated the
various unit and layout tests.

This implements (parts of) the changes to the spec made in
https://github.com/mkruisselbrink/ServiceWorker/commit/88bbe9324fa8ac38d8af3e347564450d598757aa

BUG=540509
Committed: https://crrev.com/0126c13fddde0066adf3ae782d135968b5ad8b6f
Cr-Commit-Position: refs/heads/master@{#376036}

[Marijn Kruisselbrink, 2016-02-02 19:17:14]

Description was changed from

==========
WIP Add origins argument to registerForeignFetchScopes.

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

to

==========
Add the new origins argument to registerForeignFetchScopes.

Adds the new argument, stores the list of origins for which to intercept
requests, and adds origin checks to the interception code. Also updated the
various unit and layout tests.

This implements (parts of) the changes to the spec made in
https://github.com/mkruisselbrink/ServiceWorker/commit/88bbe9324fa8ac38d8af3e...

BUG=540509
==========

[Marijn Kruisselbrink, 2016-02-02 19:17:22]

Patchset #2 (id:20001) has been deleted

[Marijn Kruisselbrink, 2016-02-02 19:17:27]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2016-02-03 00:38:42]

Patchset #1 (id:40001) has been deleted

[Marijn Kruisselbrink, 2016-02-03 00:39:10]

Description was changed from

==========
Add the new origins argument to registerForeignFetchScopes.

Adds the new argument, stores the list of origins for which to intercept
requests, and adds origin checks to the interception code. Also updated the
various unit and layout tests.

This implements (parts of) the changes to the spec made in
https://github.com/mkruisselbrink/ServiceWorker/commit/88bbe9324fa8ac38d8af3e...

BUG=540509
==========

to

==========
Add origins argument to registerForeignFetchScopes.

Adds the new argument, stores the list of origins for which to intercept
requests, and adds origin checks to the interception code. Also updated the
various unit and layout tests.

This implements (parts of) the changes to the spec made in
https://github.com/mkruisselbrink/ServiceWorker/commit/88bbe9324fa8ac38d8af3e...

BUG=540509
==========

[Marijn Kruisselbrink, 2016-02-03 00:51:19]

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

[falken, 2016-02-03 02:17:32]

lgtm

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

https://codereview.chromium.org/1656933003/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1338:
set_foreign_fetch_scopes(sub_scopes);
The previous code appended the scopes to the end, is this change intentional?

https://codereview.chromium.org/1656933003/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/1656933003/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:1143: //
Invalid URL, should kill worker (but in tests will only increase bad
nit: Now URL is ambiguous since there are both subscopes and origins. Maybe the
comments in this test would read better as "Invalid subscope" or "Invalid
origin" explicitly?

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html
(right):

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html:58:
}, 'Service Worker doesn\'t intercepts fetches with non matching origin.');
nit: intercept

https://codereview.chromium.org/1656933003/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/1656933003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js:88:
}, 'Wildcard origin string in array');
Ah, this is supposed to be the same as '*'?

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js:115:
}, 'Origins includes other strings and wildcard');
Nice thorough tests!

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/InstallEvent.cpp (right):

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/InstallEvent.cpp:48: }
I'm not sure but would ['*'] and '*' arguments have the same effect? The spec
seems to treat them differently.

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/InstallEvent.cpp:51: if
(originList.size() != 1 || originList[0] != "*") {
The '*' special case and how it translates to the empty list is somewhat
non-obvious. Would it be worth copy/pasting the spec as a comment?

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/InstallEvent.cpp:56:
exceptionState.throwTypeError("Invalid Origin URL: " + originList[i]);
nit: should be consistent about origin vs Origin in the erorr messages.

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/InstallEvent.cpp:71:
exceptionState.throwTypeError("Invalid URL: " + subScopes[i]);
nit: here too maybe it should say "Invalid subscope URL"

[Marijn Kruisselbrink, 2016-02-03 17:53:30]

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

[Marijn Kruisselbrink, 2016-02-03 17:53:31]

+tsepez for IPC OWNERS

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

https://codereview.chromium.org/1656933003/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1338:
set_foreign_fetch_scopes(sub_scopes);
On 2016/02/03 at 02:17:31, falken wrote:
> The previous code appended the scopes to the end, is this change intentional?

Yes, this change is intentional. The spec always said to replace the subscopes,
so the old implementation was just wrong (although at least when it was only
scopes it might have somewhat sense to append instead; with both scopes and
origins I think that would just be confusing).

https://codereview.chromium.org/1656933003/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/1656933003/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:1143: //
Invalid URL, should kill worker (but in tests will only increase bad
On 2016/02/03 at 02:17:31, falken wrote:
> nit: Now URL is ambiguous since there are both subscopes and origins. Maybe
the comments in this test would read better as "Invalid subscope" or "Invalid
origin" explicitly?

Done

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html
(right):

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html:58:
}, 'Service Worker doesn\'t intercepts fetches with non matching origin.');
On 2016/02/03 at 02:17:31, falken wrote:
> nit: intercept

Done

https://codereview.chromium.org/1656933003/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/1656933003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/register-foreign-fetch-errors-worker.js:88:
}, 'Wildcard origin string in array');
On 2016/02/03 at 02:17:32, falken wrote:
> Ah, this is supposed to be the same as '*'?

I think that would be the least confusing yes. But I agree that I'll need to
clarify that in the spec.

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/InstallEvent.cpp (right):

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/InstallEvent.cpp:48: }
On 2016/02/03 at 02:17:32, falken wrote:
> I'm not sure but would ['*'] and '*' arguments have the same effect? The spec
seems to treat them differently.

Hmm, yeah. I think I would prefer to treat those the same. I added a comment to
the spec pull request to figure out how to properly specify whatever the desired
behavior is and will reconcile the implementation if that turns out to be
needed.

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/InstallEvent.cpp:51: if
(originList.size() != 1 || originList[0] != "*") {
On 2016/02/03 at 02:17:32, falken wrote:
> The '*' special case and how it translates to the empty list is somewhat
non-obvious. Would it be worth copy/pasting the spec as a comment?

I added a comment to try to explain this.

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/InstallEvent.cpp:56:
exceptionState.throwTypeError("Invalid Origin URL: " + originList[i]);
On 2016/02/03 at 02:17:32, falken wrote:
> nit: should be consistent about origin vs Origin in the erorr messages.

Done

https://codereview.chromium.org/1656933003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/InstallEvent.cpp:71:
exceptionState.throwTypeError("Invalid URL: " + subScopes[i]);
On 2016/02/03 at 02:17:32, falken wrote:
> nit: here too maybe it should say "Invalid subscope URL"

Done

[Tom Sepez, 2016-02-03 18:04:09]

Messages LGTM

[Marijn Kruisselbrink, 2016-02-03 18:09:45]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-02-03 18:09:45]

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/1656933003/#ps80001
(title: "address comments")

[commit-bot: I haz the power, 2016-02-03 18:11:10]

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

[commit-bot: I haz the power, 2016-02-03 18:25:51]

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

[commit-bot: I haz the power, 2016-02-03 18:25:52]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Marijn Kruisselbrink, 2016-02-03 18:27:09]

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

[Marijn Kruisselbrink, 2016-02-03 18:27:10]

+japhet for third_party/WebKit/Source/web OWNERS

[michaeln, 2016-02-03 21:27:20]

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

[michaeln, 2016-02-03 21:27:21]

https://codereview.chromium.org/1656933003/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1656933003/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:152: const
std::vector<GURL>& foreign_fetch_origins() const {
drive by: would it make sense to / is it possible to use the url::Origin class
for this?

[Marijn Kruisselbrink, 2016-02-04 19:30:07]

https://codereview.chromium.org/1656933003/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1656933003/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:152: const
std::vector<GURL>& foreign_fetch_origins() const {
On 2016/02/03 at 21:27:21, michaeln wrote:
> drive by: would it make sense to / is it possible to use the url::Origin class
for this?

I'm not sure if it would make sense. It would certainly be possible, although
slightly clunky. There are various things missing from the url::Origin API,
which makes me wonder if something like this is what url::Origin was intended
for. One big issues is that there isn't really a way to persist a url::Origin.
It has a Serialize() method, but no easy way to recreate an Origin from that.
Although deserializing into a GURL and then converting that to a url::Origin
probably does the right thing anyway. And there are similar issues at the
blink<->content boundary. There are no conversions possible between
blink::WebSecurityOrigin and url::Origin, other than serializing to a string,
converting the string to a GURL and then creating a url::Origin from that
string.
Maybe these are just missing APIs that really should exist though, so maybe it
is worth reaching out to whoever owns the url::Origin code to figure out if
that's what I should be using here...

[Marijn Kruisselbrink, 2016-02-04 20:00:45]

https://codereview.chromium.org/1656933003/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1656933003/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:152: const
std::vector<GURL>& foreign_fetch_origins() const {
On 2016/02/04 at 19:30:07, Marijn Kruisselbrink wrote:
> On 2016/02/03 at 21:27:21, michaeln wrote:
> > drive by: would it make sense to / is it possible to use the url::Origin
class for this?
> 
> I'm not sure if it would make sense. It would certainly be possible, although
slightly clunky. There are various things missing from the url::Origin API,
which makes me wonder if something like this is what url::Origin was intended
for. One big issues is that there isn't really a way to persist a url::Origin.
It has a Serialize() method, but no easy way to recreate an Origin from that.
Although deserializing into a GURL and then converting that to a url::Origin
probably does the right thing anyway. And there are similar issues at the
blink<->content boundary. There are no conversions possible between
blink::WebSecurityOrigin and url::Origin, other than serializing to a string,
converting the string to a GURL and then creating a url::Origin from that
string.
> Maybe these are just missing APIs that really should exist though, so maybe it
is worth reaching out to whoever owns the url::Origin code to figure out if
that's what I should be using here...

Actually, the blink issues don't exist. I was looking at the wrong
WebSecurityOrigin.h file (somehow blink ended up with two copies of the class
that aren't actually identical....). So changing this probably makes sense.

[Marijn Kruisselbrink, 2016-02-04 22:40:15]

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

[Marijn Kruisselbrink, 2016-02-04 22:40:15]

Okay, changed my code to use url::Origin and blink::(Web)SecurityOrigin.

+mkwst as a different Source/web OWNER, and to make sure this is what
url::Origin can/should be used for.

For tests it's mildly annoying that url::Origin doesn't have an operator==, but
there might be a good reason why having one (that would just call
IsSameOriginWith) would be a bad idea?

[michaeln, 2016-02-05 20:23:38]

i didn't mean to add myself as a reviewer, just noticed the newer'ish Origin
class and a potential use for it, but lgtm all the same

[Mike West, 2016-02-12 09:25:19]

On 2016/02/04 at 22:40:15, mek wrote:
> Okay, changed my code to use url::Origin and blink::(Web)SecurityOrigin.
> 
> +mkwst as a different Source/web OWNER, and to make sure this is what
url::Origin can/should be used for.

LGTM. This seems totally reasonable from that perspective.

> For tests it's mildly annoying that url::Origin doesn't have an operator==,
but there might be a good reason why having one (that would just call
IsSameOriginWith) would be a bad idea?

I'm fine with that. I don't recall why we didn't add it to begin with; I think
it was just that we wanted to be explicit about the meaning of the comparison
being made (that is, two unique origins could feasibly be considered "==", but
aren't "same-origin"). That explicitness is probably outweighed by being able to
use things like test macros, though, so *shrug*.

[Marijn Kruisselbrink, 2016-02-13 02:38:58]

Patchset #5 (id:140001) has been deleted

[Marijn Kruisselbrink, 2016-02-13 02:49:24]

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

[Marijn Kruisselbrink, 2016-02-13 02:49:25]

+brettw for url/origin.h OWNERS

[Marijn Kruisselbrink, 2016-02-17 17:27:14]

On 2016/02/13 at 02:49:25, Marijn Kruisselbrink wrote:
> +brettw for url/origin.h OWNERS

brettw: ping?

[brettw, 2016-02-17 20:38:39]

origin.h lgtm

[Marijn Kruisselbrink, 2016-02-17 20:46:46]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-02-17 20:46:47]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
tsepez@chromium.org, michaeln@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1656933003/#ps160001
(title: "add url::Origin::operator== to make tests simpler")

[commit-bot: I haz the power, 2016-02-17 20:48:27]

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

[commit-bot: I haz the power, 2016-02-17 23:51:06]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2016-02-17 23:53:02]

Description was changed from

==========
Add origins argument to registerForeignFetchScopes.

Adds the new argument, stores the list of origins for which to intercept
requests, and adds origin checks to the interception code. Also updated the
various unit and layout tests.

This implements (parts of) the changes to the spec made in
https://github.com/mkruisselbrink/ServiceWorker/commit/88bbe9324fa8ac38d8af3e...

BUG=540509
==========

to

==========
Add origins argument to registerForeignFetchScopes.

Adds the new argument, stores the list of origins for which to intercept
requests, and adds origin checks to the interception code. Also updated the
various unit and layout tests.

This implements (parts of) the changes to the spec made in
https://github.com/mkruisselbrink/ServiceWorker/commit/88bbe9324fa8ac38d8af3e...

BUG=540509

Committed: https://crrev.com/0126c13fddde0066adf3ae782d135968b5ad8b6f
Cr-Commit-Position: refs/heads/master@{#376036}
==========

[commit-bot: I haz the power, 2016-02-17 23:53:03]

Patchset 5 (id:??) landed as
https://crrev.com/0126c13fddde0066adf3ae782d135968b5ad8b6f
Cr-Commit-Position: refs/heads/master@{#376036}