Reenable feature policy control over fullscreen

Reenable feature policy control over fullscreen

This CL also changes test expectations to bring the fullscreen tests in
line with the new behaviour prescribed by Feature Policy.

Specifically:
 - Same origin iframes by default have the same ability to use
   fullscreen as their parent frame. Tests which previously only used
   same-origin frame have been changed to verify the new behaviour,
   and new tests in LayoutTests/http/tests have been added to test the
   same situation with cross-origin frames.
 - Dynamic modification of the allowfullscreen flag has no effect until
   the iframe contents are navigated/reloaded.
 - Web platform tests are marked as failing, and should remain so until
   the fullscreen spec is updated fo include the new behaviour.
 - A new Browser test class is created which explicitly disables
   feature policy so that we don't lose coverage for the old behaviour
   when FP is disabled.

BUG=718155, 623682
Review-Url: https://codereview.chromium.org/2898503002
Cr-Commit-Position: refs/heads/master@{#495331}
Committed: https://chromium.googlesource.com/chromium/src/+/5d8010e1fc081481d0646618e700b51a4699ab4c

[iclelland, 2017-05-19 20:44:25]

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

[commit-bot: I haz the power, 2017-05-19 20:45:33]

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 21:10:18]

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

[commit-bot: I haz the power, 2017-05-19 21:10:18]

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

[iclelland, 2017-05-25 20:51:14]

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

[commit-bot: I haz the power, 2017-05-25 20:52:06]

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-25 21:21:42]

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

[commit-bot: I haz the power, 2017-05-25 21:21:43]

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

[iclelland, 2017-06-27 20:45:32]

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

[commit-bot: I haz the power, 2017-06-27 20:45:51]

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-06-27 21:53:52]

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

[commit-bot: I haz the power, 2017-06-27 21:53:53]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[iclelland, 2017-07-04 15:48:19]

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

[commit-bot: I haz the power, 2017-07-04 15:48:35]

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-07-04 17:38:04]

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

[commit-bot: I haz the power, 2017-07-04 17:38:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[iclelland, 2017-07-04 19:38:54]

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

[commit-bot: I haz the power, 2017-07-04 19:39:05]

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-07-04 21:52:46]

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

[commit-bot: I haz the power, 2017-07-04 21:52:47]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-08-03 14:51:53]

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

[commit-bot: I haz the power, 2017-08-03 14:51:57]

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-08-03 16:03:49]

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

[commit-bot: I haz the power, 2017-08-03 16:03:50]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[iclelland, 2017-08-04 18:21:19]

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

[commit-bot: I haz the power, 2017-08-04 18:21:29]

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-08-04 19:43:17]

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

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

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

[iclelland, 2017-08-08 15:22:46]

Description was changed from

==========
Reenable feature policy control over fullscreen

BUG=718155,623682
==========

to

==========
Reenable feature policy control over fullscreen

This CL also changes test expectations to bring the fullscreen tests in
line with the new behaviour prescribed by Feature Policy.

Specifically:
 - Same origin iframes by default have the same ability to use
   fullscreen as their parent frame. Tests which previously only used
   same-origin frame have been changed to verify the new behaviour,
   and new tests in LayoutTests/http/tests have been added to test the
   same situation with cross-origin frames.
 - Dynamic modification of the allowfullscreen flag has no effect until
   the iframe contents are navigated/reloaded.
 - Web platform tests are marked as failing, and should remain so until
   the fullscreen spec is updated fo include the new behaviour.
 - A new Browser test class is created which explicitly disables
   feature policy so that we don't lose coverage for the old behaviour
   when FP is disabled.

BUG=718155,623682
==========

[iclelland, 2017-08-08 15:22:47]

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

[iclelland, 2017-08-08 15:24:17]

+r foolip -- can you take a look at this? I'd like to get the tests in a state
where we can try using FP for fullscreen.

Thanks!

[iclelland, 2017-08-08 15:43:13]

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

[iclelland, 2017-08-08 15:43:15]

Also +r looneybear, can you PTAL?

[loonybear, 2017-08-08 17:55:55]

Hi Ian,

Thanks for doing this. This lgtm.
At some point, do we want to create some wpt tests for fullscreen
enabled/disabled by FP too?

Thanks,
Luna

https://codereview.chromium.org/2898503002/diff/120001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2898503002/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:8715: // fullscreen if the
allowFullscreen attribute is present in all of its ancestor
Just to clarify, a frame is allowed to enter fullscreen only if all of its
ancestor frames have "allowfullscreen" attribute right? If one of his ancestor
does not or removes it, the iframe will no longer be able to enter fullscreen
right?

https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (left):

https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2889: crbug.com/737643
http/tests/feature-policy/payment-enabledforall.php [ Failure ]
Not related to this CL, but at some point, maybe we can remove all the virtual
tests since similar stuff is being tested in wpt?

Although if we are allowing different vendors to enable different features with
FP first, we might still keep our own version of LayoutTests, WDYT?

[iclelland, 2017-08-08 20:14:26]

> At some point, do we want to create some wpt tests for fullscreen
> enabled/disabled by FP too?

Yes, but that's not until the actual spec is updated, I think.

https://codereview.chromium.org/2898503002/diff/120001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2898503002/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:8715: // fullscreen if the
allowFullscreen attribute is present in all of its ancestor
On 2017/08/08 17:55:53, loonybear wrote:
> Just to clarify, a frame is allowed to enter fullscreen only if all of its
> ancestor frames have "allowfullscreen" attribute right? If one of his ancestor
> does not or removes it, the iframe will no longer be able to enter fullscreen
> right? 

That's correct; that's the original behaviour of allowfullscreen. (And that's
why this is tested in the FeaturePolicyDisabledBrowserTest)

https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (left):

https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2889: crbug.com/737643
http/tests/feature-policy/payment-enabledforall.php [ Failure ]
On 2017/08/08 17:55:53, loonybear wrote:
> Not related to this CL, but at some point, maybe we can remove all the virtual
> tests since similar stuff is being tested in wpt?

I sure hope so -- we could probably get rid of the feature-policy virtual test
suite now, since the basic mechanism is turned on by default now.

> Although if we are allowing different vendors to enable different features
with
> FP first, we might still keep our own version of LayoutTests, WDYT?

Probably -- with fullscreen, though, I think that we want to update the spec to
say that, even without FP, the same-origin case and the dynamic
allowfullscreen-attribute behaviour should be changed. If we can get that change
into the spec, then we can land WPT tests for it which don't require supporting
feature policy at all.

[foolip, 2017-08-09 09:00:49]

https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2879: # Feature Policy changes
fullscreen behaviour, tests need updating
Can you update these tests as part of this review? wpt is the main test suite
I'm using for Fullscreen development, so keeping them passing is equally
important to the others.

[iclelland, 2017-08-09 15:10:36]

https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2879: # Feature Policy changes
fullscreen behaviour, tests need updating
On 2017/08/09 09:00:49, foolip wrote:
> Can you update these tests as part of this review? wpt is the main test suite
> I'm using for Fullscreen development, so keeping them passing is equally
> important to the others.

Sure, I can do that. I didn't because I thought the flow was:
1. Behaviour change breaks wpt tests.
2. Spec change triggers new wpt tests.
3. Tests pass again.

This way would be
1. Behaviour change triggers new wpt tests; wpt out of sync with spec (and other
browsers).
2. Spec changes to re-align with new behaviour; back in sync with wpt tests.

As long as that's the desired flow, I can do that.

[iclelland, 2017-08-09 15:12:38]

https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2879: # Feature Policy changes
fullscreen behaviour, tests need updating
On 2017/08/09 15:10:35, iclelland wrote:
> On 2017/08/09 09:00:49, foolip wrote:
> > Can you update these tests as part of this review? wpt is the main test
suite
> > I'm using for Fullscreen development, so keeping them passing is equally
> > important to the others.
> 
> Sure, I can do that. I didn't because I thought the flow was:
> 1. Behaviour change breaks wpt tests.
> 2. Spec change triggers new wpt tests.
> 3. Tests pass again.
> 
> This way would be
> 1. Behaviour change triggers new wpt tests; wpt out of sync with spec (and
other
> browsers).
> 2. Spec changes to re-align with new behaviour; back in sync with wpt tests.
> 
> As long as that's the desired flow, I can do that.

Another way to do this would be to just update the wpt tests to avoid the
behaviour that's changing -- i.e, don't test same-origin frames -- change all of
those tests to cross-origin ones. Then when the spec changes, add explicit tests
for same-origin behaviour to wpt. WDYT?

[foolip, 2017-08-09 15:38:43]

https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2879: # Feature Policy changes
fullscreen behaviour, tests need updating
On 2017/08/09 15:12:38, iclelland wrote:
> On 2017/08/09 15:10:35, iclelland wrote:
> > On 2017/08/09 09:00:49, foolip wrote:
> > > Can you update these tests as part of this review? wpt is the main test
> suite
> > > I'm using for Fullscreen development, so keeping them passing is equally
> > > important to the others.
> > 
> > Sure, I can do that. I didn't because I thought the flow was:
> > 1. Behaviour change breaks wpt tests.
> > 2. Spec change triggers new wpt tests.
> > 3. Tests pass again.
> > 
> > This way would be
> > 1. Behaviour change triggers new wpt tests; wpt out of sync with spec (and
> other
> > browsers).
> > 2. Spec changes to re-align with new behaviour; back in sync with wpt tests.
> > 
> > As long as that's the desired flow, I can do that.
> 
> Another way to do this would be to just update the wpt tests to avoid the
> behaviour that's changing -- i.e, don't test same-origin frames -- change all
of
> those tests to cross-origin ones. Then when the spec changes, add explicit
tests
> for same-origin behaviour to wpt. WDYT?

Yes, something like that. How about:

Create a new document-fullscreen-enabled-cross-origin.html with cross-origin
documents that is per the current spec and will be passing. And allow the
current test to start failing, but by adding
document-fullscreen-enabled-expected.txt if possible.

The situation for element-ready-check-enabled-flag-not-set-manual.html will be
the same, split it into two tests where one is failing.

Then, the spec change will go together with changes to the two original tests
which will cause them to begin passing again.

(If you like, it'd be fine to add tests in the same file instead, preferable
even if it allows for any code reuse.)

[iclelland, 2017-08-11 18:00:57]

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

[commit-bot: I haz the power, 2017-08-11 18:01:08]

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-08-11 19:02:22]

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

[commit-bot: I haz the power, 2017-08-11 19:02:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[iclelland, 2017-08-14 16:08:43]

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

[commit-bot: I haz the power, 2017-08-14 16:08:55]

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-08-14 17:52:07]

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

[commit-bot: I haz the power, 2017-08-14 17:52:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[iclelland, 2017-08-14 18:05:00]

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

[commit-bot: I haz the power, 2017-08-14 18:05:07]

Dry run: CQ is trying da patch.

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

[iclelland, 2017-08-14 18:11:11]

On 2017/08/09 15:38:43, foolip wrote:
>
https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2898503002/diff/120001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/TestExpectations:2879: # Feature Policy changes
> fullscreen behaviour, tests need updating
> On 2017/08/09 15:12:38, iclelland wrote:
> > On 2017/08/09 15:10:35, iclelland wrote:
> > > On 2017/08/09 09:00:49, foolip wrote:
> > > > Can you update these tests as part of this review? wpt is the main test
> > suite
> > > > I'm using for Fullscreen development, so keeping them passing is equally
> > > > important to the others.
> > > 
> > > Sure, I can do that. I didn't because I thought the flow was:
> > > 1. Behaviour change breaks wpt tests.
> > > 2. Spec change triggers new wpt tests.
> > > 3. Tests pass again.
> > > 
> > > This way would be
> > > 1. Behaviour change triggers new wpt tests; wpt out of sync with spec (and
> > other
> > > browsers).
> > > 2. Spec changes to re-align with new behaviour; back in sync with wpt
tests.
> > > 
> > > As long as that's the desired flow, I can do that.
> > 
> > Another way to do this would be to just update the wpt tests to avoid the
> > behaviour that's changing -- i.e, don't test same-origin frames -- change
all
> of
> > those tests to cross-origin ones. Then when the spec changes, add explicit
> tests
> > for same-origin behaviour to wpt. WDYT?
> 
> Yes, something like that. How about:
> 
> Create a new document-fullscreen-enabled-cross-origin.html with cross-origin
> documents that is per the current spec and will be passing. And allow the
> current test to start failing, but by adding
> document-fullscreen-enabled-expected.txt if possible.

Done -- PTAL to double-check that what I've done matches what you were expecting
:)
> 
> The situation for element-ready-check-enabled-flag-not-set-manual.html will be
> the same, split it into two tests where one is failing.

Also done.

> Then, the spec change will go together with changes to the two original tests
> which will cause them to begin passing again.
> (If you like, it'd be fine to add tests in the same file instead, preferable
> even if it allows for any code reuse.)

Yes, with the spec update, we'll be able to add additional test cases to the new
files; they support multiple subtests with different frame arguments and
different expectations.

[commit-bot: I haz the power, 2017-08-14 19:58:01]

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

[commit-bot: I haz the power, 2017-08-14 19:58:04]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[foolip, 2017-08-15 09:44:23]

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html:6:
<!-- iframe src="resources/report-full-screen-enabled.html"
name="same-origin-default"></iframe -->
Why is this test excluded? If it's because it would fail, can you instead loop
over expectations and create one async_test per case? These should all be OK to
run in parallel, which would simplify somewhat too.

If that covers the same ground as document-fullscreen-enabled.html, you could
just replace that test with this one, no need to keep redundant coverage.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html:7:
<iframe src="resources/report-full-screen-enabled.html" allowfullscreen
name="same-origin-allow"></iframe>
s/full-screen/fullscreen/ in these file names,
https://fullscreen.spec.whatwg.org/ doesn't have the FullScreen naming
variations (yet)

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html:26:
t.step(() => {
Rather than just wrapping the asserts, can you t.step_func(e => { ... }) as the
event listener?

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-cross-origin-manual.sub.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-cross-origin-manual.sub.html:12:
//   "Frame name": [fullscreen-enabled-flag, did-enter-fullscreen]
The fullscreenEnabled check in the original test was just a precondition really,
since it's now adding more than a single line, I think it's fine to omit it, and
depend on the other test for that.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-cross-origin-manual.sub.html:14:
"cross-origin-default": [false, false],
Can you either put the two tests in one file, or simplify this away? I think I'd
lean towards keeping them separate and simplifying this test, because when you
make the FP changes to Fullscreen, what's currently
element-ready-check-enabled-flag-not-set-manual.html won't make much sense, and
should perhaps be renamed to element-request-fullscreen-iframe-manual.html or
something, not merged into this test.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-cross-origin-manual.sub.html:17:
assert_array_equals = (expected, actual, prefix) => {
The assert_array_equals in testharness.js does about the same thing and includes
the description in the failure messages, any reason to replace it?

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-manual-expected.txt
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-manual-expected.txt:2:
FAIL Element ready check with enabled flag not set assert_false: fullscreen
enabled flag expected false got true
The name of this test refers to the "fullscreen enabled flag" which was removed
in https://github.com/whatwg/html/pull/1492.

Can you rename the result to element-ready-check-not-allowed-manual.html to
match how this is currently phrased in the spec? (Elsewhere there's the question
of one file vs. two files.)

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/attempt-full-screen.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/attempt-full-screen.html:21:
document.body.requestFullscreen();
This request ought to always fail since it's not in response to a user gesture,
or possibly it depends on the timeout for user gestures?

Can this be changed to click the iframe instead? I think that this file would
have to include trusted-click.js, since the button has to be in this iframe. And
then there's the problem that uto-click.js not working.

Alternatively, if this really isn't failing due to the user gesture, that could
be proven by having another test that successfully enters fullscreen in a
cross-origin iframe with the same postMessage mechanism. Thoughts?

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/report-full-screen-enabled.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/report-full-screen-enabled.html:6:
reportFullscreenEnabled = (frame) => {
I think this can be simplified. Per
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5303 it looks like
the iframe's name correctly propagates to window.name, so you can use that. Even
without it, it should be possible to use e.source, just a bit less convenient.

In other words, why can't this script immediately post back, without even
waiting for the page to finish loading?

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/report-full-screen-enabled.html:9:
"enabled": document.webkitFullscreenEnabled,
Use document.fullscreenEnabled in wpt.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/fullscreen/full-screen-enabled.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fullscreen/full-screen-enabled.html:5:
<title>fullscreenEnabled test</title>
These duplicated tests aren't necessary IMHO, the API surface is mostly aliases,
with differences only in which events fire, so if you've tested the unprefixed
API well, that's enough.

[iclelland, 2017-08-16 13:31:26]

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

[commit-bot: I haz the power, 2017-08-16 13:31:41]

Dry run: CQ is trying da patch.

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

[iclelland, 2017-08-16 13:31:56]

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html:6:
<!-- iframe src="resources/report-full-screen-enabled.html"
name="same-origin-default"></iframe -->
On 2017/08/15 09:44:22, foolip wrote:
> Why is this test excluded? If it's because it would fail, can you instead loop
> over expectations and create one async_test per case? These should all be OK
to
> run in parallel, which would simplify somewhat too.
> 

I commented it because I thought the idea was that the new tests only tested
behaviour which was consistent between the current spec and the
feature-policy-changes. I can certainly reinstate it, and include the new
expectations, if that's the right thing to do.


> If that covers the same ground as document-fullscreen-enabled.html, you could
> just replace that test with this one, no need to keep redundant coverage.

The biggest difference is that document-fullscreen-enabled.html accesses the
properties directly from the parent frame through
HTMLIFrameElement.contentDocument -- if that's not a meaningful distinction, or
a case which needs to be tested specifically for fullscreen, then that can be
deleted.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html:7:
<iframe src="resources/report-full-screen-enabled.html" allowfullscreen
name="same-origin-allow"></iframe>
On 2017/08/15 09:44:22, foolip wrote:
> s/full-screen/fullscreen/ in these file names,
> https://fullscreen.spec.whatwg.org/ doesn't have the FullScreen naming
> variations (yet)

Done.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html:26:
t.step(() => {
On 2017/08/15 09:44:22, foolip wrote:
> Rather than just wrapping the asserts, can you t.step_func(e => { ... }) as
the
> event listener?

Yes, yes it turns out I can. Thanks :)

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-cross-origin-manual.sub.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-cross-origin-manual.sub.html:12:
//   "Frame name": [fullscreen-enabled-flag, did-enter-fullscreen]
On 2017/08/15 09:44:22, foolip wrote:
> The fullscreenEnabled check in the original test was just a precondition
really,
> since it's now adding more than a single line, I think it's fine to omit it,
and
> depend on the other test for that.

Removed. Now just checks did-enter-fullscreen.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-cross-origin-manual.sub.html:14:
"cross-origin-default": [false, false],
On 2017/08/15 09:44:22, foolip wrote:
> Can you either put the two tests in one file, or simplify this away?

I'm not 100% certain what you mean by this, but I'm parsing it as "Can you
either combine this test with the one in
document-fullscreen-enabled-cross-origin.sub.html, or remove the
fullscreen-enabled check from this test?"

If so, I think you're right, and I should just remove the fullscreen-enabled
test here, and only test didEnterFullscreen.

> I think I'd
> lean towards keeping them separate and simplifying this test, because when you
> make the FP changes to Fullscreen, what's currently
> element-ready-check-enabled-flag-not-set-manual.html won't make much sense,
and
> should perhaps be renamed to element-request-fullscreen-iframe-manual.html or
> something, not merged into this test.

Done. (unless there were further simplifications you were thinking of)

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-cross-origin-manual.sub.html:17:
assert_array_equals = (expected, actual, prefix) => {
On 2017/08/15 09:44:22, foolip wrote:
> The assert_array_equals in testharness.js does about the same thing and
includes
> the description in the failure messages, any reason to replace it?

None. Not sure why I couldn't see it there. Unneeded now, but I'll remember for
next time that it's in there.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-manual-expected.txt
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-manual-expected.txt:2:
FAIL Element ready check with enabled flag not set assert_false: fullscreen
enabled flag expected false got true
On 2017/08/15 09:44:22, foolip wrote:
> The name of this test refers to the "fullscreen enabled flag" which was
removed
> in https://github.com/whatwg/html/pull/1492.
> 
> Can you rename the result to element-ready-check-not-allowed-manual.html to
> match how this is currently phrased in the spec? (Elsewhere there's the
question
> of one file vs. two files.)

Done. This is the pre-existing test, but I've also renamed the new one.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/attempt-full-screen.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/attempt-full-screen.html:21:
document.body.requestFullscreen();
On 2017/08/15 09:44:22, foolip wrote:
> This request ought to always fail since it's not in response to a user
gesture,
> or possibly it depends on the timeout for user gestures?

Interesting... this is triggered by postmessage from the parent within a click,
so it should work. (At least it appears to, and it actually goes full-screen in
chrome if I add 'allowfullscreen' to the iframe tag)
> 
> Can this be changed to click the iframe instead? I think that this file would
> have to include trusted-click.js, since the button has to be in this iframe.
And
> then there's the problem that uto-click.js not working.
> 
> Alternatively, if this really isn't failing due to the user gesture, that
could
> be proven by having another test that successfully enters fullscreen in a
> cross-origin iframe with the same postMessage mechanism. Thoughts?

I can extend it to run a second test, but I'll have to re-work the event
handlers in the parent page.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/report-full-screen-enabled.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/report-full-screen-enabled.html:6:
reportFullscreenEnabled = (frame) => {
On 2017/08/15 09:44:22, foolip wrote:
> I think this can be simplified. Per
> http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5303 it looks like
> the iframe's name correctly propagates to window.name, so you can use that.
Even
> without it, it should be possible to use e.source, just a bit less convenient.
> 
> In other words, why can't this script immediately post back, without even
> waiting for the page to finish loading?

That's interesting... If the frame name is available ambiently, then we
definitely can just post back, and don't need to trigger. The trigger was just
so that we could pass a token to the child frame to allow the results to be
distinguished properly on the return message.

In the other test, which actually enables fullscreen, I can remove the need to
pass frame.name back and forth, but the trigger is still necessary in order to
satisfy the gesture requirement.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/report-full-screen-enabled.html:9:
"enabled": document.webkitFullscreenEnabled,
On 2017/08/15 09:44:22, foolip wrote:
> Use document.fullscreenEnabled in wpt.

Thanks, done.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/fullscreen/full-screen-enabled.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fullscreen/full-screen-enabled.html:5:
<title>fullscreenEnabled test</title>
On 2017/08/15 09:44:23, foolip wrote:
> These duplicated tests aren't necessary IMHO, the API surface is mostly
aliases,
> with differences only in which events fire, so if you've tested the unprefixed
> API well, that's enough.

Removed. The unprefixed tests remain in /external/wpt. If there are other tests
which you think are redundant, I'll remove those as well.

[iclelland, 2017-08-16 14:20:57]

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/attempt-full-screen.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/attempt-full-screen.html:21:
document.body.requestFullscreen();
On 2017/08/16 13:31:55, iclelland wrote:
> On 2017/08/15 09:44:22, foolip wrote:
> > This request ought to always fail since it's not in response to a user
> gesture,
> > or possibly it depends on the timeout for user gestures?
> 
> Interesting... this is triggered by postmessage from the parent within a
click,
> so it should work. (At least it appears to, and it actually goes full-screen
in
> chrome if I add 'allowfullscreen' to the iframe tag)
> > 
> > Can this be changed to click the iframe instead? I think that this file
would
> > have to include trusted-click.js, since the button has to be in this iframe.
> And
> > then there's the problem that uto-click.js not working.
> > 
> > Alternatively, if this really isn't failing due to the user gesture, that
> could
> > be proven by having another test that successfully enters fullscreen in a
> > cross-origin iframe with the same postMessage mechanism. Thoughts?
> 
> I can extend it to run a second test, but I'll have to re-work the event
> handlers in the parent page.

And this is done; I've split the test into two, to avoid having to do a bunch of
work serializing the tests, since the test cases have to be run one after the
other (unlike the fullscreenEnabled flag check).

[commit-bot: I haz the power, 2017-08-16 15:03:32]

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

[commit-bot: I haz the power, 2017-08-16 15:03:33]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-08-16 15:12:34]

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

[iclelland, 2017-08-16 15:12:35]

Also +r alexmos@ -- can you approve the change in
site_per_process_browsertest.cc?

Essentially, I want a way to continue to test the behaviour of allowfullscreen
in the case where feature policy is disabled, until we completely remove the old
code paths.

[foolip, 2017-08-16 21:43:15]

lgtm % nits

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html:6:
<!-- iframe src="resources/report-full-screen-enabled.html"
name="same-origin-default"></iframe -->
On 2017/08/16 13:31:55, iclelland wrote:
> On 2017/08/15 09:44:22, foolip wrote:
> > Why is this test excluded? If it's because it would fail, can you instead
loop
> > over expectations and create one async_test per case? These should all be OK
> to
> > run in parallel, which would simplify somewhat too.
> > 
> 
> I commented it because I thought the idea was that the new tests only tested
> behaviour which was consistent between the current spec and the
> feature-policy-changes. I can certainly reinstate it, and include the new
> expectations, if that's the right thing to do.

Right, the tests should match what the spec currently says (so one will fail in
Chromium) but AFAICT it's still fine to have them all in a single file, with the
4 permutations of same/cross-origin and allowfullscreen attribute or not.

> > If that covers the same ground as document-fullscreen-enabled.html, you
could
> > just replace that test with this one, no need to keep redundant coverage.
> 
> The biggest difference is that document-fullscreen-enabled.html accesses the
> properties directly from the parent frame through
> HTMLIFrameElement.contentDocument -- if that's not a meaningful distinction,
or
> a case which needs to be tested specifically for fullscreen, then that can be
> deleted.

Yeah, I don't think that matters, it's fine if postMessage is used even where it
doesn't have to be. Lots of other tests try reaching into same-origin iframes
and the fullscreen-related attributes aren't exotic like some [CrossOrigin] in
Window.idl, so it's fine to delete.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-cross-origin-manual.sub.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-cross-origin-manual.sub.html:14:
"cross-origin-default": [false, false],
On 2017/08/16 13:31:55, iclelland wrote:
> On 2017/08/15 09:44:22, foolip wrote:
> > Can you either put the two tests in one file, or simplify this away?
> 
> I'm not 100% certain what you mean by this, but I'm parsing it as "Can you
> either combine this test with the one in
> document-fullscreen-enabled-cross-origin.sub.html, or remove the
> fullscreen-enabled check from this test?"
> 
> If so, I think you're right, and I should just remove the fullscreen-enabled
> test here, and only test didEnterFullscreen.

Words are hard, sorry. I was wondering if a single
element-ready-check-enabled-flag-not-set.html (but renamed) or keeping them
separate makes the most sense. And, if they're not merged into one, some
simplifications follow here.

> > I think I'd
> > lean towards keeping them separate and simplifying this test, because when
you
> > make the FP changes to Fullscreen, what's currently
> > element-ready-check-enabled-flag-not-set-manual.html won't make much sense,
> and
> > should perhaps be renamed to element-request-fullscreen-iframe-manual.html
or
> > something, not merged into this test.
> 
> Done. (unless there were further simplifications you were thinking of)

Thanks!

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/attempt-full-screen.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/attempt-full-screen.html:21:
document.body.requestFullscreen();
On 2017/08/16 14:20:57, iclelland wrote:
> On 2017/08/16 13:31:55, iclelland wrote:
> > On 2017/08/15 09:44:22, foolip wrote:
> > > This request ought to always fail since it's not in response to a user
> > gesture,
> > > or possibly it depends on the timeout for user gestures?
> > 
> > Interesting... this is triggered by postmessage from the parent within a
> click,
> > so it should work. (At least it appears to, and it actually goes full-screen
> in
> > chrome if I add 'allowfullscreen' to the iframe tag)
> > > 
> > > Can this be changed to click the iframe instead? I think that this file
> would
> > > have to include trusted-click.js, since the button has to be in this
iframe.
> > And
> > > then there's the problem that uto-click.js not working.
> > > 
> > > Alternatively, if this really isn't failing due to the user gesture, that
> > could
> > > be proven by having another test that successfully enters fullscreen in a
> > > cross-origin iframe with the same postMessage mechanism. Thoughts?
> > 
> > I can extend it to run a second test, but I'll have to re-work the event
> > handlers in the parent page.
> 
> And this is done; I've split the test into two, to avoid having to do a bunch
of
> work serializing the tests, since the test cases have to be run one after the
> other (unlike the fullscreenEnabled flag check).

Ah, so I guess it works because there was a user gesture in the last few hundred
milliseconds, or whatever the rule is. I don't know how interoperable that
timeout is, but we'll find out by writing tests that depend on it :)

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/fullscreen/full-screen-enabled.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fullscreen/full-screen-enabled.html:5:
<title>fullscreenEnabled test</title>
On 2017/08/16 13:31:56, iclelland wrote:
> On 2017/08/15 09:44:23, foolip wrote:
> > These duplicated tests aren't necessary IMHO, the API surface is mostly
> aliases,
> > with differences only in which events fire, so if you've tested the
unprefixed
> > API well, that's enough.
> 
> Removed. The unprefixed tests remain in /external/wpt. If there are other
tests
> which you think are redundant, I'll remove those as well.

I didn't look super hard, but I suspect getting rid of
full-screen-iframe-legacy.html is fine too, video.webkitEnterFullScreen is
almost a plain alias, and for the purposes of FP the difference between going
fullscreen with a div or a video don't matter, I'd wager.

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2875: crbug.com/718155
fullscreen/full-screen-iframe-not-allowed.html [ Failure ]
Are these planned for a follow-up? They're not in wpt, so just changing them
before the spec changes are done is fine.

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html
(right):

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html:20:
async_test((t) => {
Because there's just one async_test, there's just one FAIL line in the
-expected.txt. If any of the following start failing, it won't show up as a
regression. Can you turn it inside out?

for (var name in expectations) {
  async_test(..., 'Bla bla ' + name);
}

Or any variation of that, like assigning new async_test('description ' + name)
to some state, if you'd like to keep a single event listener.

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html:28:
e.data.report.frame + " frame webkitFullscreenEnabled");
more webkit on the loose :)

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-not-allowed-manual.html
(right):

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-not-allowed-manual.html:2:
<title>Element ready check with enabled flag not set</title>
Nit: can you change the title of this to "with no allowfullscreen attribute" or
similar, something matching the other test.

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/attempt-fullscreen.html
(right):

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/attempt-fullscreen.html:18:
document.exitFullscreen();
I suppose it was annoying to be left in fullscreen in manual testing. A bunch of
tests do that and I haven't bothered to fix it. To make sure that exiting cannot
in any way interfere with the test outcome, you can instead use t.add_cleanup(()
=> document.exitFullscreen()) in the test file itself. Feel free to sprinkle
that one-liner in any tests you find with that problem.

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/fullscreen/full-screen-iframe-legacy.html
(right):

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fullscreen/full-screen-iframe-legacy.html:10:
<p>HTMLVideoElement.webkitEnterFullScreen() in a cross-origin iframe requires
the allowfullscreen attribute</p>
As mentioned elsewhere, I think it's OK to not cover video.webkitEnterFullScreen
specifically.

[alexmos, 2017-08-16 22:12:16]

site_per_process_browsertest.cc LGTM

https://codereview.chromium.org/2898503002/diff/200001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2898503002/diff/200001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:8923: // the change is
replicated to the child frame and its proxies.
It might be useful to say explicitly that this is now testing deprecated
behavior that will be eventually removed once Feature Policy launches, and give
pointers to bugs that explain changes in the new path.

Once FP launches, this test will presumably need to be refactored to not assume
that dynamic changes to allowfullscreen take place immediately?  Are there any
other changes that will break this test?

[iclelland, 2017-08-17 20:01:56]

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html:6:
<!-- iframe src="resources/report-full-screen-enabled.html"
name="same-origin-default"></iframe -->
On 2017/08/16 21:43:14, foolip wrote:
> On 2017/08/16 13:31:55, iclelland wrote:
> > On 2017/08/15 09:44:22, foolip wrote:
> > > Why is this test excluded? If it's because it would fail, can you instead
> loop
> > > over expectations and create one async_test per case? These should all be
OK
> > to
> > > run in parallel, which would simplify somewhat too.
> > > 
> > 
> > I commented it because I thought the idea was that the new tests only tested
> > behaviour which was consistent between the current spec and the
> > feature-policy-changes. I can certainly reinstate it, and include the new
> > expectations, if that's the right thing to do.
> 
> Right, the tests should match what the spec currently says (so one will fail
in
> Chromium) but AFAICT it's still fine to have them all in a single file, with
the
> 4 permutations of same/cross-origin and allowfullscreen attribute or not.
> 
> > > If that covers the same ground as document-fullscreen-enabled.html, you
> could
> > > just replace that test with this one, no need to keep redundant coverage.
> > 
> > The biggest difference is that document-fullscreen-enabled.html accesses the
> > properties directly from the parent frame through
> > HTMLIFrameElement.contentDocument -- if that's not a meaningful distinction,
> or
> > a case which needs to be tested specifically for fullscreen, then that can
be
> > deleted.
> 
> Yeah, I don't think that matters, it's fine if postMessage is used even where
it
> doesn't have to be. Lots of other tests try reaching into same-origin iframes
> and the fullscreen-related attributes aren't exotic like some [CrossOrigin] in
> Window.idl, so it's fine to delete.

Done.

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/fullscreen/full-screen-enabled.html
(right):

https://codereview.chromium.org/2898503002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fullscreen/full-screen-enabled.html:5:
<title>fullscreenEnabled test</title>
On 2017/08/16 21:43:14, foolip wrote:
> On 2017/08/16 13:31:56, iclelland wrote:
> > On 2017/08/15 09:44:23, foolip wrote:
> > > These duplicated tests aren't necessary IMHO, the API surface is mostly
> > aliases,
> > > with differences only in which events fire, so if you've tested the
> unprefixed
> > > API well, that's enough.
> > 
> > Removed. The unprefixed tests remain in /external/wpt. If there are other
> tests
> > which you think are redundant, I'll remove those as well.
> 
> I didn't look super hard, but I suspect getting rid of
> full-screen-iframe-legacy.html is fine too, video.webkitEnterFullScreen is
> almost a plain alias, and for the purposes of FP the difference between going
> fullscreen with a div or a video don't matter, I'd wager.

Done.

https://codereview.chromium.org/2898503002/diff/200001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2898503002/diff/200001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:8923: // the change is
replicated to the child frame and its proxies.
On 2017/08/16 22:12:16, alexmos wrote:
> It might be useful to say explicitly that this is now testing deprecated
> behavior that will be eventually removed once Feature Policy launches, and
give
> pointers to bugs that explain changes in the new path.
> 
> Once FP launches, this test will presumably need to be refactored to not
assume
> that dynamic changes to allowfullscreen take place immediately?  Are there any
> other changes that will break this test?

There are two changes that could break tests -- the fact that allowfullscreen is
no longer dynamic, and the fact that fullscreen is allowed by default in
same-origin frames -- this test only relies on the first. If we want to keep the
test, we'll have to update it to look more like
SitePerProcessFeaturePolicyBrowserTest.ContainerPolicyDynamic.

There are a number of other tests (in the SitePerProcessFeaturePolicyBrowserTest
class) which test things like policy replication, and we have unit tests that
ensure that the correct policy is constructed in the first place, so we may not
need this special-case-test for allowfullscreen at all once the old paths have
been removed.

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2875: crbug.com/718155
fullscreen/full-screen-iframe-not-allowed.html [ Failure ]
On 2017/08/16 21:43:15, foolip wrote:
> Are these planned for a follow-up? They're not in wpt, so just changing them
> before the spec changes are done is fine.

Yes, the plan is to get to changing (or removing) them as needed. I'll make sure
those are changed or removed before closing the bug out.

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html
(right):

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html:20:
async_test((t) => {
On 2017/08/16 21:43:15, foolip wrote:
> Because there's just one async_test, there's just one FAIL line in the
> -expected.txt. If any of the following start failing, it won't show up as a
> regression. Can you turn it inside out?
> 
> for (var name in expectations) {
>   async_test(..., 'Bla bla ' + name);
> }
> 
> Or any variation of that, like assigning new async_test('description ' + name)
> to some state, if you'd like to keep a single event listener.

Done.

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html:28:
e.data.report.frame + " frame webkitFullscreenEnabled");
On 2017/08/16 21:43:15, foolip wrote:
> more webkit on the loose :)

Ack! Fixed, thanks :)
(Thankfully just in text output this time)

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/attempt-fullscreen.html
(right):

https://codereview.chromium.org/2898503002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/resources/attempt-fullscreen.html:18:
document.exitFullscreen();
On 2017/08/16 21:43:15, foolip wrote:
> I suppose it was annoying to be left in fullscreen in manual testing. A bunch
of
> tests do that and I haven't bothered to fix it. To make sure that exiting
cannot
> in any way interfere with the test outcome, you can instead use
t.add_cleanup(()
> => document.exitFullscreen()) in the test file itself. Feel free to sprinkle
> that one-liner in any tests you find with that problem.

Done.

[iclelland, 2017-08-17 20:02:11]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2017-08-17 20:02:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
loonybear@chromium.org, alexmos@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2898503002/#ps220001
(title: "Fixing last test nits")

[commit-bot: I haz the power, 2017-08-17 20:02:22]

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-08-17 21:47:14]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1503000131165170,
"parent_rev": "81565fa69c0d5cdd4275fcf9d858f92b5e8aa551", "commit_rev":
"5d8010e1fc081481d0646618e700b51a4699ab4c"}

[commit-bot: I haz the power, 2017-08-17 21:47:37]

Description was changed from

==========
Reenable feature policy control over fullscreen

This CL also changes test expectations to bring the fullscreen tests in
line with the new behaviour prescribed by Feature Policy.

Specifically:
 - Same origin iframes by default have the same ability to use
   fullscreen as their parent frame. Tests which previously only used
   same-origin frame have been changed to verify the new behaviour,
   and new tests in LayoutTests/http/tests have been added to test the
   same situation with cross-origin frames.
 - Dynamic modification of the allowfullscreen flag has no effect until
   the iframe contents are navigated/reloaded.
 - Web platform tests are marked as failing, and should remain so until
   the fullscreen spec is updated fo include the new behaviour.
 - A new Browser test class is created which explicitly disables
   feature policy so that we don't lose coverage for the old behaviour
   when FP is disabled.

BUG=718155,623682
==========

to

==========
Reenable feature policy control over fullscreen

This CL also changes test expectations to bring the fullscreen tests in
line with the new behaviour prescribed by Feature Policy.

Specifically:
 - Same origin iframes by default have the same ability to use
   fullscreen as their parent frame. Tests which previously only used
   same-origin frame have been changed to verify the new behaviour,
   and new tests in LayoutTests/http/tests have been added to test the
   same situation with cross-origin frames.
 - Dynamic modification of the allowfullscreen flag has no effect until
   the iframe contents are navigated/reloaded.
 - Web platform tests are marked as failing, and should remain so until
   the fullscreen spec is updated fo include the new behaviour.
 - A new Browser test class is created which explicitly disables
   feature policy so that we don't lose coverage for the old behaviour
   when FP is disabled.

BUG=718155,623682

Review-Url: https://codereview.chromium.org/2898503002
Cr-Commit-Position: refs/heads/master@{#495331}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d8010e1fc081481d0646618e700...
==========

[commit-bot: I haz the power, 2017-08-17 21:47:39]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/5d8010e1fc081481d0646618e700...