third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html - Issue 2898503002: Reenable feature policy control over fullscreen

Side by Side Diff: third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-cross-origin.sub.html

Issue 2898503002: Reenable feature policy control over fullscreen (Closed)

Patch Set: Reinstate original (same-origin) wpt tests with failing expectations Created 3 years, 4 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« no previous file with comments | « third_party/WebKit/LayoutTests/TestExpectations ('k') | third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/document-fullscreen-enabled-expected.txt » ('j') | third_party/WebKit/LayoutTests/external/wpt/fullscreen/api/element-ready-check-enabled-flag-not-set-cross-origin-manual.sub.html » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <title>Document#fullscreenEnabled</title>

	3 <script src="/resources/testharness.js"></script>

	4 <script src="/resources/testharnessreport.js"></script>

	5 <div id="log"></div>

	6 <!-- iframe src="resources/report-full-screen-enabled.html" name="same-origin-de fault"></iframe -->
	foolip 2017/08/15 09:44:22 Why is this test excluded? If it's because it woul 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. iclelland 2017/08/16 13:31:55 I commented it because I thought the idea was that Show quoted text 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. Show quoted text > 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. foolip 2017/08/16 21:43:14 Right, the tests should match what the spec curren Show quoted text 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. Show quoted text > > 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. iclelland 2017/08/17 20:01:56 Done. Show quoted text 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.
	7 <iframe src="resources/report-full-screen-enabled.html" allowfullscreen name="sa me-origin-allow"></iframe>
	foolip 2017/08/15 09:44:22 s/full-screen/fullscreen/ in these file names, htt s/full-screen/fullscreen/ in these file names, https://fullscreen.spec.whatwg.org/ doesn't have the FullScreen naming variations (yet) iclelland 2017/08/16 13:31:55 Done. Show quoted text 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.
	8 <iframe src="http://{{domains[www1]}}:{{ports[http][0]}}/fullscreen/api/resource s/report-full-screen-enabled.html" name="cross-origin-default"></iframe>

	9 <iframe src="http://{{domains[www1]}}:{{ports[http][0]}}/fullscreen/api/resource s/report-full-screen-enabled.html" allowfullscreen name="cross-origin-allow"></i frame>

	10 <script>

	11 // Fullscreen is enabled in same-origin frames regardless of allowfullscreen,

	12 // but must be explicitly enabled in cross-origin frames.

	13 var expectations = {

	14 // "same-origin-default": true,

	15 "same-origin-allow": true,

	16 "cross-origin-default": false,

	17 "cross-origin-allow": true

	18 };

	19

	20 async_test((t) => {

	21 // When a message is received from a child frame, ensure that the report

	22 // matches the expectations.

	23 window.addEventListener('message', (e) => {

	24 if (e.data.report && e.data.report.api == "fullscreen") {

	25 if (e.data.report.frame in expectations) {

	26 t.step(() => {
	foolip 2017/08/15 09:44:22 Rather than just wrapping the asserts, can you t.s Rather than just wrapping the asserts, can you t.step_func(e => { ... }) as the event listener? iclelland 2017/08/16 13:31:55 Yes, yes it turns out I can. Thanks :) Show quoted text 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 :)
	27 assert_equals(e.data.report.enabled,

	28 expectations[e.data.report.frame],

	29 e.data.report.frame + " frame webkitFullscreenEnabled"); });

	30 delete expectations[e.data.report.frame];

	31 // When all child frames have reported, the test is complete.

	32 if (Object.keys(expectations).length == 0)

	33 t.done();

	34 }

	35 }

	36 });

	37

	38 // Trigger the child frames to report as soon as their content is loaded.

	39 document.querySelectorAll('iframe').forEach((elem) => {

	40 elem.addEventListener('load', () => {

	41 elem.contentWindow.postMessage({"action": "report", "frame": elem.name},

	42 "*");

	43 });

	44 });

	45 });

	46 </script>

OLD	NEW