Add LayoutTest for ServiceWorker's sandbox iframe handling.

Add LayoutTest for ServiceWorker's sandbox iframe handling.

This is a test for https://codereview.chromium.org/1191293002/

BUG=486308
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197892

[horo, 2015-06-25 04:03:17]

Patchset #1 (id:1) has been deleted

[horo, 2015-06-25 05:07:43]

Patchset #1 (id:20001) has been deleted

[horo, 2015-06-25 05:08:35]

horo@chromium.org changed reviewers:
+ nhiroki@chromium.org

[horo, 2015-06-25 05:08:36]

nhiroki@
Could you please review this?

[nhiroki, 2015-06-26 01:01:37]

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/chromium/resources/sandboxed-iframe-fetch-event-iframe.html
(right):

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/resources/sandboxed-iframe-fetch-event-iframe.html:38:
function() {
Why don't you use 'catch'?

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/chromium/resources/sandboxed-iframe-fetch-event-worker.js
(right):

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/resources/sandboxed-iframe-fetch-event-worker.js:7:
for(var i of clients){
super-nit: "c" would be more natural that "i"

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-fetch-event.html
(right):

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-fetch-event.html:82:
assert_equals(requests[4], expected_base_url + '?script-origin');
Adding comments (or 3rd parameter to assert_equals) about test expectations
could be helpful to read. For example,

- sandboxed iframe w/ "allow-same-origin" should be intercepted by fetch handler
and treated as a client
- sandboxed iframe w/o "allow-same-origin" should not be ...

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-fetch-event.html:88:
expected_base_url + '?script-origin_script-origin');
nit: Can you add a blank line after line 88 for readability?

[horo, 2015-06-26 03:59:11]

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/chromium/resources/sandboxed-iframe-fetch-event-iframe.html
(right):

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/resources/sandboxed-iframe-fetch-event-iframe.html:38:
function() {
On 2015/06/26 01:01:36, nhiroki wrote:
> Why don't you use 'catch'?

Done.

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/chromium/resources/sandboxed-iframe-fetch-event-worker.js
(right):

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/resources/sandboxed-iframe-fetch-event-worker.js:7:
for(var i of clients){
On 2015/06/26 01:01:36, nhiroki wrote:
> super-nit: "c" would be more natural that "i"

Done.

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-fetch-event.html
(right):

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-fetch-event.html:82:
assert_equals(requests[4], expected_base_url + '?script-origin');
On 2015/06/26 01:01:37, nhiroki wrote:
> Adding comments (or 3rd parameter to assert_equals) about test expectations
> could be helpful to read. For example,
> 
> - sandboxed iframe w/ "allow-same-origin" should be intercepted by fetch
handler
> and treated as a client
> - sandboxed iframe w/o "allow-same-origin" should not be ...

Done.

https://codereview.chromium.org/1208693003/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-fetch-event.html:88:
expected_base_url + '?script-origin_script-origin');
On 2015/06/26 01:01:37, nhiroki wrote:
> nit: Can you add a blank line after line 88 for readability?

Done.

[nhiroki, 2015-06-26 04:33:32]

lgtm with one optional comment

https://codereview.chromium.org/1208693003/diff/80001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-fetch-event.html
(right):

https://codereview.chromium.org/1208693003/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-fetch-event.html:80:
!!request_set[expected_base_url + '?iframe'],
You might want to use 'in' operator like this:

  expected_base_url + '?iframe' in request_set

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/in

[horo, 2015-06-26 04:57:25]

Thank you!

https://codereview.chromium.org/1208693003/diff/80001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-fetch-event.html
(right):

https://codereview.chromium.org/1208693003/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-fetch-event.html:80:
!!request_set[expected_base_url + '?iframe'],
On 2015/06/26 04:33:32, nhiroki wrote:
> You might want to use 'in' operator like this:
> 
>   expected_base_url + '?iframe' in request_set
> 
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/in

Done.

[horo, 2015-06-26 05:00:47]

The CQ bit was checked by horo@chromium.org

[horo, 2015-06-26 05:00:48]

The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/1208693003/#ps100001
(title: "use in")

[commit-bot: I haz the power, 2015-06-26 05:01:03]

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

[commit-bot: I haz the power, 2015-06-26 06:00:35]

Committed patchset #4 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197892