Re-enable feature policy layout tests after bug fix.

Re-enable feature policy layout tests after bug fix.

Tests previously failing on site isolation bots.
Reconstructed all feature policy layout tests:
  - replaced loading multiple testharnessreport.js by introducing postMessage's
  - formatted all tests in more consistent template

BUG=716085
Review-Url: https://codereview.chromium.org/2855133004
Cr-Commit-Position: refs/heads/master@{#474299}
Committed: https://chromium.googlesource.com/chromium/src/+/0f559bdba5bf694e3d7ad34cd90b316ac6736d95

[lunalu1, 2017-05-03 21:36:58]

lunalu@chromium.org changed reviewers:
+ iclelland@chromium.org

[lunalu1, 2017-05-03 21:36:59]

Hi Ian, 
Sorry this CL is giant. 

Most tests use the same template so you can just comment on one file and skip
the rest and I will readdress the comments on all files

Thanks!

[Charlie Reis, 2017-05-03 21:42:46]

Description was changed from

==========
Re-enable feature policy layout tests after bug fix.

Tests previously failing on site isolation bots.
Reconstructed all feature policy layout tests:
  - replaced loading multiple testharnessreport.js by introducing postMessage's
  - formatted all tests in more consistent template

BUG=716085
==========

to

==========
Re-enable feature policy layout tests after bug fix.

Tests previously failing on site isolation bots.
Reconstructed all feature policy layout tests:
  - replaced loading multiple testharnessreport.js by introducing postMessage's
  - formatted all tests in more consistent template

BUG=716085
==========

[lunalu1, 2017-05-04 14:54:25]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 14:54:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 16:10:39]

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

[commit-bot: I haz the power, 2017-05-04 16:10:39]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[iclelland, 2017-05-18 19:36:13]

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy-relocate-and-no-reload.html
(right):

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy-relocate-and-no-reload.html:24:
}, 'iframe relocated without reload, allow = vibrate, is disabled on ' +
I think a clearer message here might be something like "Iframe navigated itself
to a new origin, vibrate is disabled by container policy"

I don't think that 'relocate' is a term used anywhere else.

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy-relocate-and-reload.html
(right):

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy-relocate-and-reload.html:27:
}, 'iframe reloaded to URL: ' + src + ', allow = vibrate, is enabled on ' +
And here, maybe instead of "reloaded to URL", we could say "iframe src set to "

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy.html
(right):

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy.html:34:
loadFrame(src);
Will/could this set the iframe src attribute a second time before the first
document loads into it? Or does the promise_test have to completely resolve
before loadFrame returns?

If the promise_test runs asynchronously, then maybe what we need is two separate
iframe tags?

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-disabled-expected.txt
(right):

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-disabled-expected.txt:2:
FAIL Navigator.vibrate disabled on URL: resources/feature-policy-vibrate.html
assert_false: navigator.vibrate(): expected false got true
Does this mean that the test is currently failing? That vibrate is being allowed
everywhere, including cross-origin frames, even when disabled via feature
policy?

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-allowed-by-container-policy-relocate-expected.txt
(right):

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-allowed-by-container-policy-relocate-expected.txt:2:
FAIL iframe relocated to URL:
http://localhost:8000/feature-policy/resources/feature-policy-fullscreen.html
with allowpaymentrequest = falseallow = fullscreen is enabled by container
policy. assert_true: Document.fullscreenEnabled: expected true got false
Is there a space missing in the middle of "falseallow" there? It looks like it
should be there, according to the test, I can't see why it would be missing in
the output.

[iclelland, 2017-05-18 19:40:59]

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy.html
(right):

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy.html:10:
function sendClick() {
This function could probably be moved into a little javascript library, I don't
know if it's worth it, but it does seem to be used a lot in the vibrate tests

[iclelland, 2017-05-18 19:56:59]

A couple more small nits:

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy.html
(right):

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy.html:34:
loadFrame(src);
On 2017/05/18 19:36:12, iclelland wrote:
> Will/could this set the iframe src attribute a second time before the first
> document loads into it? Or does the promise_test have to completely resolve
> before loadFrame returns?
> 
> If the promise_test runs asynchronously, then maybe what we need is two
separate
> iframe tags?

I had a look, to re-learn how promise_test worked, and I can see that they are
all queued to run sequentially, and since src is a parameter here, that should
work.

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-allowed-by-container-policy-relocate.html
(right):

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-allowed-by-container-policy-relocate.html:14:
var allowfullscreen = iframe.allowFullscreen;
allowfullscreen => allowFullscreen

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-allowed-by-container-policy-relocate.html:20:
var enabled =
I think we should call this something like "expectedEnabled", or
"shouldBeEnabled"

[lunalu1, 2017-05-19 18:20:36]

Patchset #3 (id:40001) has been deleted

[lunalu1, 2017-05-19 20:53:42]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2017-05-19 20:54:08]

Done. PTAL
Thanks

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy-relocate-and-no-reload.html
(right):

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy-relocate-and-no-reload.html:24:
}, 'iframe relocated without reload, allow = vibrate, is disabled on ' +
On 2017/05/18 at 19:36:12, iclelland wrote:
> I think a clearer message here might be something like "Iframe navigated
itself to a new origin, vibrate is disabled by container policy"
> 
> I don't think that 'relocate' is a term used anywhere else.

Done

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy-relocate-and-reload.html
(right):

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy-relocate-and-reload.html:27:
}, 'iframe reloaded to URL: ' + src + ', allow = vibrate, is enabled on ' +
On 2017/05/18 at 19:36:12, iclelland wrote:
> And here, maybe instead of "reloaded to URL", we could say "iframe src set to
"

Done.

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy.html
(right):

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy.html:10:
function sendClick() {
On 2017/05/18 at 19:40:59, iclelland wrote:
> This function could probably be moved into a little javascript library, I
don't know if it's worth it, but it does seem to be used a lot in the vibrate
tests

Done

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy.html:34:
loadFrame(src);
On 2017/05/18 19:56:58, iclelland wrote:
> On 2017/05/18 19:36:12, iclelland wrote:
> > Will/could this set the iframe src attribute a second time before the first
> > document loads into it? Or does the promise_test have to completely resolve
> > before loadFrame returns?
> > 
> > If the promise_test runs asynchronously, then maybe what we need is two
> separate
> > iframe tags?
> 
> I had a look, to re-learn how promise_test worked, and I can see that they are
> all queued to run sequentially, and since src is a parameter here, that should
> work.

:)

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-disabled-expected.txt
(right):

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-disabled-expected.txt:2:
FAIL Navigator.vibrate disabled on URL: resources/feature-policy-vibrate.html
assert_false: navigator.vibrate(): expected false got true
On 2017/05/18 at 19:36:12, iclelland wrote:
> Does this mean that the test is currently failing? That vibrate is being
allowed everywhere, including cross-origin frames, even when disabled via
feature policy?

This is no longer the case.

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-allowed-by-container-policy-relocate.html
(right):

https://codereview.chromium.org/2855133004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-allowed-by-container-policy-relocate.html:20:
var enabled =
On 2017/05/18 at 19:56:58, iclelland wrote:
> I think we should call this something like "expectedEnabled", or
"shouldBeEnabled"

Done

[commit-bot: I haz the power, 2017-05-19 20:54:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 22:17:04]

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

[commit-bot: I haz the power, 2017-05-19 22:17:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[iclelland, 2017-05-23 14:15:03]

Looks like some test expectation files still need to be updated here, but this
is looking great, thanks!

I'm curious to know why the vibrate tests are failing (failng to parse the
policy), in the feature-policy-experimental-features test suite -- do you know
why that is?

https://codereview.chromium.org/2855133004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy/payment-allowed-by-container-policy.html
(right):

https://codereview.chromium.org/2855133004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy/payment-allowed-by-container-policy.html:29:
loadIframes(src);
Should be srcs?

[lunalu1, 2017-05-23 17:42:43]

On 2017/05/23 at 14:15:03, iclelland wrote:
> Looks like some test expectation files still need to be updated here, but this
is looking great, thanks!
> 
> I'm curious to know why the vibrate tests are failing (failng to parse the
policy), in the feature-policy-experimental-features test suite -- do you know
why that is?
> 
>
https://codereview.chromium.org/2855133004/diff/60001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/http/tests/feature-policy/payment-allowed-by-container-policy.html
(right):
> 
>
https://codereview.chromium.org/2855133004/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/feature-policy/payment-allowed-by-container-policy.html:29:
loadIframes(src);
> Should be srcs?

Yes. That was a typo. Done.

[lunalu1, 2017-05-23 18:55:56]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 18:56:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-05-23 19:41:07]

Done. Please take another look.

Because there is a console warning for
vibrate-allowed-by-container-policy-reloacte-and-no-reload, even though the test
passes, a test expect is still required.

Re to vibrate not allowed behind the experimental-features flag, I can't
reproduce it. If you are talking about the case for
vibrate-allowed-by-container-policy-reloacte-and-no-reload, it is disallowed due
to feature policy: when relocate without reload, the inherited-policy doesn't
get updated from "allowed" features.

Thanks

[lunalu1, 2017-05-23 20:19:04]

Somehow the tests results for vibrate does not match my local version. I thinks
something needs to be changed in HTMLIFrameElementAllow

Let me investigate more first before you review it.

Thanks Ian

[commit-bot: I haz the power, 2017-05-23 20:28:52]

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

[commit-bot: I haz the power, 2017-05-23 20:28:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2017-05-23 20:58:41]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 20:59:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-05-23 20:59:37]

All test expects updated. Tests behave as expected. Please take another look.

Thanks

[commit-bot: I haz the power, 2017-05-23 22:29:28]

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

[commit-bot: I haz the power, 2017-05-23 22:29:30]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-05-24 14:48:30]

lgtm

[lunalu1, 2017-05-24 15:35:41]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2017-05-24 15:35:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 15:41:00]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495640141330190,
"parent_rev": "7682c6e584d676e2f002ab15aa8480df383ac0d5", "commit_rev":
"0f559bdba5bf694e3d7ad34cd90b316ac6736d95"}

[commit-bot: I haz the power, 2017-05-24 15:41:15]

Description was changed from

==========
Re-enable feature policy layout tests after bug fix.

Tests previously failing on site isolation bots.
Reconstructed all feature policy layout tests:
  - replaced loading multiple testharnessreport.js by introducing postMessage's
  - formatted all tests in more consistent template

BUG=716085
==========

to

==========
Re-enable feature policy layout tests after bug fix.

Tests previously failing on site isolation bots.
Reconstructed all feature policy layout tests:
  - replaced loading multiple testharnessreport.js by introducing postMessage's
  - formatted all tests in more consistent template

BUG=716085

Review-Url: https://codereview.chromium.org/2855133004
Cr-Commit-Position: refs/heads/master@{#474299}
Committed:
https://chromium.googlesource.com/chromium/src/+/0f559bdba5bf694e3d7ad34cd90b...
==========

[commit-bot: I haz the power, 2017-05-24 15:41:18]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0f559bdba5bf694e3d7ad34cd90b...