Foreign fetch should not intercept same origin or main resource requests.

Foreign fetch should not intercept same origin or main resource requests.

As specified in step 4 and 9 of https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on-foreign-fetch-request-algorithm

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

[Marijn Kruisselbrink, 2016-04-07 19:07:28]

Description was changed from

==========
Foreign fetch should not intercept same origin or main resource requests.

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

to

==========
Foreign fetch should not intercept same origin or main resource requests.

As specified in step 4 and 9 of
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...

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

[Marijn Kruisselbrink, 2016-04-07 19:08:19]

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

[falken, 2016-04-08 01:28:29]

Looks good. I may just be missing it but where is the test for the behavior
change? It looks like foreign-fetch-basics.html is just refactoring existing
tests.

https://codereview.chromium.org/1870743002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/install-worker-helper.html
(right):

https://codereview.chromium.org/1870743002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/install-worker-helper.html:6:
navigator.serviceWorker.register(event.data.worker, event.data.options)
We usually unregister before register to be resilient to mess from other tests.

https://codereview.chromium.org/1870743002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/install-worker-helper.html:15:
});
Can you unregister these workers when the test ends?

[Marijn Kruisselbrink, 2016-04-08 17:20:37]

On 2016/04/08 at 01:28:29, falken wrote:
> Looks good. I may just be missing it but where is the test for the behavior
change? It looks like foreign-fetch-basics.html is just refactoring existing
tests.

Good point. I added a test to make sure same origin requests are not
intercepted.

https://codereview.chromium.org/1870743002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/install-worker-helper.html
(right):

https://codereview.chromium.org/1870743002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/install-worker-helper.html:6:
navigator.serviceWorker.register(event.data.worker, event.data.options)
On 2016/04/08 at 01:28:29, falken wrote:
> We usually unregister before register to be resilient to mess from other
tests.

Good point, added an unregister call before the register.

https://codereview.chromium.org/1870743002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/install-worker-helper.html:15:
});
On 2016/04/08 at 01:28:29, falken wrote:
> Can you unregister these workers when the test ends?

I added an unload handler on the iframe to unregister the worker when the iframe
is destroyed.

[falken, 2016-04-14 01:22:52]

Sorry for the extreme delay. Is it possible to add a test for main resource
request as well?

It'd be nice if deleting the IsMainResourceType check would cause a test to
fail.

Otherwise lgtm.

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

https://codereview.chromium.org/1870743002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html:84:
var scope = 'resources/foreign-fetch/smae-origin';
nit: same-origin

[Marijn Kruisselbrink, 2016-04-15 22:35:20]

On 2016/04/14 at 01:22:52, falken wrote:
> Sorry for the extreme delay. Is it possible to add a test for main resource
request as well?
> 
> It'd be nice if deleting the IsMainResourceType check would cause a test to
fail.

The fact that it has to be a cross origin navigation (for the same origin check
to not already catch it) makes it a bit tricky to test, but of course it's a
good thing to test for. So added a test to cover this.

> Otherwise lgtm.
> 
>
https://codereview.chromium.org/1870743002/diff/60001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html
(right):
> 
>
https://codereview.chromium.org/1870743002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html:84:
var scope = 'resources/foreign-fetch/smae-origin';
> nit: same-origin

Fixed

[falken, 2016-04-18 03:13:51]

lgtm

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

https://codereview.chromium.org/1870743002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/foreign-fetch-worker.js:15:
function handleBasic(event) {
these should be w3c_style_function_names

https://codereview.chromium.org/1870743002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/foreign-fetch-worker.js:24:
{headers: {'Content-Type': 'text/html', 'Access-Control-Allow-Origin': '*'}})});
I didn't realize the hoops you had to jump through to test navigation
specifically. Thanks for writing the test!

[Marijn Kruisselbrink, 2016-04-18 18:19:20]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-04-18 18:19:21]

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

https://codereview.chromium.org/1870743002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/foreign-fetch-worker.js:15:
function handleBasic(event) {
On 2016/04/18 at 03:13:51, falken wrote:
> these should be w3c_style_function_names

Done (fwiw fetch-event-test-worker has the same incorrectly styled function
names).

[Marijn Kruisselbrink, 2016-04-18 18:19:21]

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/1870743002/#ps100001
(title: "fix function name style")

[commit-bot: I haz the power, 2016-04-18 18:19:43]

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

[commit-bot: I haz the power, 2016-04-18 19:25:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-18 19:26:34]

Description was changed from

==========
Foreign fetch should not intercept same origin or main resource requests.

As specified in step 4 and 9 of
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...

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

to

==========
Foreign fetch should not intercept same origin or main resource requests.

As specified in step 4 and 9 of
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...

BUG=540509

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

[commit-bot: I haz the power, 2016-04-18 19:26:35]

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