Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2564543002:
Add tests for fullscreenchange and fullscreenerror event timing (Closed)
DescriptionAdd tests for fullscreenchange and fullscreenerror event timing
Although HTML does not yet define animation frame tasks as used by
Fullscreen, it does have "run the fullscreen rendering steps". That's
the old hook, making the timing of these events relative to resize and
animation frame callbacks unambiguous:
https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen
https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model
Spec issue: https://github.com/whatwg/html/issues/707
document-exit-fullscreen-timing-manual.html consistently matches
-expected.txt, with (accidentally) correct timing but no resize event.
element-request-fullscreen-timing-manual.html usually matches
-expected.txt, with incorrect timing, but can accidentally get the
timing right and instead fail due to no resize event.
Both are marked as flaky to be safe, and will be enabled in a coming CL.
Note: This CL is also a test for the WPT export process.
Firefox passes both fullscreechange event tests, but fails the
fullscreenerror test. (Implementation preceded spec changes.)
BUG=402376, 672436
Committed: https://crrev.com/9eef72d72ab21ad9d7513a3bc6a649fd3cc1d685
Cr-Commit-Position: refs/heads/master@{#437283}
Patch Set 1 #
Total comments: 7
Patch Set 2 : assert resize timing #
Messages
Total messages: 31 (15 generated)
The CQ bit was checked by foolip@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
foolip@chromium.org changed reviewers: + jeffcarp@chromium.org, mlamouri@chromium.org
mlamouri@, PTAL. These tests will be enabled by https://codereview.chromium.org/2557943002/ which I hope to also send out soon. I've filed https://crbug.com/672436 for the resize event issue. I've verified that the tests pass in Chrome (where the resize event is fired) after https://codereview.chromium.org/2557943002/ and https://codereview.chromium.org/2536643002 added unit tests for the timing, so I think this is OK and I won't block anything on fixing it. jeffcarp@, this is an exportable commit for you to test with. I will land it ASAP, and if it is accidentally reverted by import that's OK as long as it eventually comes back.
Found https://github.com/whatwg/html/issues/707 where I just proposed how to sort this out spec-side.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html (right): https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html:23: console.info('resize event timing not tested'); Should that fail? https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/element-request-fullscreen-timing-manual.html (right): https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/element-request-fullscreen-timing-manual.html:18: console.info('resize event timing not tested'); ditto -- based on the explanations above, shouldn't that be an assertion?
https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html (right): https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html:23: console.info('resize event timing not tested'); On 2016/12/08 14:23:24, mlamouri wrote: > Should that fail? I did it this way because of https://crbug.com/672436 But sure, this is for upstream WPT and this'll look strange, so I'll make the test fail instead, and make a copy of the test in the coming CL that skips the resize check with reference to that bug.
https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html (right): https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html:23: console.info('resize event timing not tested'); On 2016/12/08 at 14:33:03, foolip wrote: > On 2016/12/08 14:23:24, mlamouri wrote: > > Should that fail? > > I did it this way because of https://crbug.com/672436 > > But sure, this is for upstream WPT and this'll look strange, so I'll make the test fail instead, and make a copy of the test in the coming CL that skips the resize check with reference to that bug. You can also have an -expected.txt file if you have expected failures.
https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html (right): https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html:23: console.info('resize event timing not tested'); On 2016/12/08 15:12:10, mlamouri wrote: > On 2016/12/08 at 14:33:03, foolip wrote: > > On 2016/12/08 14:23:24, mlamouri wrote: > > > Should that fail? > > > > I did it this way because of https://crbug.com/672436 > > > > But sure, this is for upstream WPT and this'll look strange, so I'll make the > test fail instead, and make a copy of the test in the coming CL that skips the > resize check with reference to that bug. > > You can also have an -expected.txt file if you have expected failures. Yes, but this failure is early, and so would mask later failures. I'll try to instead record the order of events and assert_array_equals after the setTimeout instead, that should change the expected output if the first element of the array changes.
https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html (right): https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html:23: console.info('resize event timing not tested'); On 2016/12/08 at 15:16:57, foolip wrote: > On 2016/12/08 15:12:10, mlamouri wrote: > > On 2016/12/08 at 14:33:03, foolip wrote: > > > On 2016/12/08 14:23:24, mlamouri wrote: > > > > Should that fail? > > > > > > I did it this way because of https://crbug.com/672436 > > > > > > But sure, this is for upstream WPT and this'll look strange, so I'll make the > > test fail instead, and make a copy of the test in the coming CL that skips the > > resize check with reference to that bug. > > > > You can also have an -expected.txt file if you have expected failures. > > Yes, but this failure is early, and so would mask later failures. I'll try to instead record the order of events and assert_array_equals after the setTimeout instead, that should change the expected output if the first element of the array changes. In case of it can help, testharness.js has an `EventWatcher`.
https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html (right): https://codereview.chromium.org/2564543002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html:23: console.info('resize event timing not tested'); On 2016/12/08 15:21:55, mlamouri wrote: > On 2016/12/08 at 15:16:57, foolip wrote: > > On 2016/12/08 15:12:10, mlamouri wrote: > > > On 2016/12/08 at 14:33:03, foolip wrote: > > > > On 2016/12/08 14:23:24, mlamouri wrote: > > > > > Should that fail? > > > > > > > > I did it this way because of https://crbug.com/672436 > > > > > > > > But sure, this is for upstream WPT and this'll look strange, so I'll make > the > > > test fail instead, and make a copy of the test in the coming CL that skips > the > > > resize check with reference to that bug. > > > > > > You can also have an -expected.txt file if you have expected failures. > > > > Yes, but this failure is early, and so would mask later failures. I'll try to > instead record the order of events and assert_array_equals after the setTimeout > instead, that should change the expected output if the first element of the > array changes. > > In case of it can help, testharness.js has an `EventWatcher`. Yeah, I've been wanting to make use of that, but have had to stay away for many fullscreen tests because it uses promises and thus would get in the way of verifying the exact timing of when events are fired, which is the point of this test. I'm sure there are some fullscreen tests that I could simplify with EventWatcher though, and leave the exact timing to tests like this.
The CQ bit was checked by foolip@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add tests for fullscreenchange and fullscreenerror event timing Although HTML does not yet define animation frame tasks as used by Fullscreen, it does have "run the fullscreen rendering steps". That's the old hook, making the timing of these events relative to resize and animation frame callbacks unambiguous: https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-... The tested order is not currently guaranteed, although the test can pass anyway. document-exit-fullscreen-timing-manual.html appears to pass consistently, while element-request-fullscreen-timing-manual.html passes about 10% of runs. Both are marked as flaky to be safe, and will be enabled in a coming CL. Note: This CL is also a test for the WPT export process. BUG=402376 ========== to ========== Add tests for fullscreenchange and fullscreenerror event timing Although HTML does not yet define animation frame tasks as used by Fullscreen, it does have "run the fullscreen rendering steps". That's the old hook, making the timing of these events relative to resize and animation frame callbacks unambiguous: https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-... The tested order is not currently guaranteed, although the test can pass anyway. document-exit-fullscreen-timing-manual.html appears to pass consistently, while element-request-fullscreen-timing-manual.html passes about 10% of runs. Both are marked as flaky to be safe, and will be enabled in a coming CL. Note: This CL is also a test for the WPT export process. BUG=402376,672436 ==========
Description was changed from ========== Add tests for fullscreenchange and fullscreenerror event timing Although HTML does not yet define animation frame tasks as used by Fullscreen, it does have "run the fullscreen rendering steps". That's the old hook, making the timing of these events relative to resize and animation frame callbacks unambiguous: https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-... The tested order is not currently guaranteed, although the test can pass anyway. document-exit-fullscreen-timing-manual.html appears to pass consistently, while element-request-fullscreen-timing-manual.html passes about 10% of runs. Both are marked as flaky to be safe, and will be enabled in a coming CL. Note: This CL is also a test for the WPT export process. BUG=402376,672436 ========== to ========== Add tests for fullscreenchange and fullscreenerror event timing Although HTML does not yet define animation frame tasks as used by Fullscreen, it does have "run the fullscreen rendering steps". That's the old hook, making the timing of these events relative to resize and animation frame callbacks unambiguous: https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-... Spec issue: https://github.com/whatwg/html/issues/707 document-exit-fullscreen-timing-manual.html appears to consistently match -expected.txt, with (accidentally) correct timing but no resize event. element-request-fullscreen-timing-manual.html fails due to incorrect timing per -expected.txt, but can accidentally get the timing right and instead fail due to no resize event. Both are marked as flaky to be safe, and will be enabled in a coming CL. Note: This CL is also a test for the WPT export process. BUG=402376,672436 ==========
The tests are now on a form where the order is tested last, so that they don't need to be duplicated. Description is also updated. Can you take another look?
Description was changed from ========== Add tests for fullscreenchange and fullscreenerror event timing Although HTML does not yet define animation frame tasks as used by Fullscreen, it does have "run the fullscreen rendering steps". That's the old hook, making the timing of these events relative to resize and animation frame callbacks unambiguous: https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-... Spec issue: https://github.com/whatwg/html/issues/707 document-exit-fullscreen-timing-manual.html appears to consistently match -expected.txt, with (accidentally) correct timing but no resize event. element-request-fullscreen-timing-manual.html fails due to incorrect timing per -expected.txt, but can accidentally get the timing right and instead fail due to no resize event. Both are marked as flaky to be safe, and will be enabled in a coming CL. Note: This CL is also a test for the WPT export process. BUG=402376,672436 ========== to ========== Add tests for fullscreenchange and fullscreenerror event timing Although HTML does not yet define animation frame tasks as used by Fullscreen, it does have "run the fullscreen rendering steps". That's the old hook, making the timing of these events relative to resize and animation frame callbacks unambiguous: https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-... Spec issue: https://github.com/whatwg/html/issues/707 document-exit-fullscreen-timing-manual.html consistently matches -expected.txt, with (accidentally) correct timing but no resize event. element-request-fullscreen-timing-manual.html usually matches -expected.txt, with incorrect timing, but can accidentally get the timing right and instead fail due to no resize event. Both are marked as flaky to be safe, and will be enabled in a coming CL. Note: This CL is also a test for the WPT export process. Firefox passes both fullscreechange event tests, but fails the fullscreenerror test. (Implementation preceded spec changes.) BUG=402376,672436 ==========
still lgtm. Thanks! :)
The CQ bit was unchecked by foolip@chromium.org
The CQ bit was checked by foolip@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
On 2016/12/08 at 17:40:48, jeffcarp wrote: > lgtm Just to confirm, you expect only these files to be exported from this commit, right: third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/element-request-fullscreen-timing-manual.html
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481218078695010,
"parent_rev": "8065ed17ea4802b916c4b43615945a303f3d7512", "commit_rev":
"c438735fa00768ba50eaa0c5528029918ad6de39"}
Message was sent while issue was closed.
Description was changed from ========== Add tests for fullscreenchange and fullscreenerror event timing Although HTML does not yet define animation frame tasks as used by Fullscreen, it does have "run the fullscreen rendering steps". That's the old hook, making the timing of these events relative to resize and animation frame callbacks unambiguous: https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-... Spec issue: https://github.com/whatwg/html/issues/707 document-exit-fullscreen-timing-manual.html consistently matches -expected.txt, with (accidentally) correct timing but no resize event. element-request-fullscreen-timing-manual.html usually matches -expected.txt, with incorrect timing, but can accidentally get the timing right and instead fail due to no resize event. Both are marked as flaky to be safe, and will be enabled in a coming CL. Note: This CL is also a test for the WPT export process. Firefox passes both fullscreechange event tests, but fails the fullscreenerror test. (Implementation preceded spec changes.) BUG=402376,672436 ========== to ========== Add tests for fullscreenchange and fullscreenerror event timing Although HTML does not yet define animation frame tasks as used by Fullscreen, it does have "run the fullscreen rendering steps". That's the old hook, making the timing of these events relative to resize and animation frame callbacks unambiguous: https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-... Spec issue: https://github.com/whatwg/html/issues/707 document-exit-fullscreen-timing-manual.html consistently matches -expected.txt, with (accidentally) correct timing but no resize event. element-request-fullscreen-timing-manual.html usually matches -expected.txt, with incorrect timing, but can accidentally get the timing right and instead fail due to no resize event. Both are marked as flaky to be safe, and will be enabled in a coming CL. Note: This CL is also a test for the WPT export process. Firefox passes both fullscreechange event tests, but fails the fullscreenerror test. (Implementation preceded spec changes.) BUG=402376,672436 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add tests for fullscreenchange and fullscreenerror event timing Although HTML does not yet define animation frame tasks as used by Fullscreen, it does have "run the fullscreen rendering steps". That's the old hook, making the timing of these events relative to resize and animation frame callbacks unambiguous: https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-... Spec issue: https://github.com/whatwg/html/issues/707 document-exit-fullscreen-timing-manual.html consistently matches -expected.txt, with (accidentally) correct timing but no resize event. element-request-fullscreen-timing-manual.html usually matches -expected.txt, with incorrect timing, but can accidentally get the timing right and instead fail due to no resize event. Both are marked as flaky to be safe, and will be enabled in a coming CL. Note: This CL is also a test for the WPT export process. Firefox passes both fullscreechange event tests, but fails the fullscreenerror test. (Implementation preceded spec changes.) BUG=402376,672436 ========== to ========== Add tests for fullscreenchange and fullscreenerror event timing Although HTML does not yet define animation frame tasks as used by Fullscreen, it does have "run the fullscreen rendering steps". That's the old hook, making the timing of these events relative to resize and animation frame callbacks unambiguous: https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-... Spec issue: https://github.com/whatwg/html/issues/707 document-exit-fullscreen-timing-manual.html consistently matches -expected.txt, with (accidentally) correct timing but no resize event. element-request-fullscreen-timing-manual.html usually matches -expected.txt, with incorrect timing, but can accidentally get the timing right and instead fail due to no resize event. Both are marked as flaky to be safe, and will be enabled in a coming CL. Note: This CL is also a test for the WPT export process. Firefox passes both fullscreechange event tests, but fails the fullscreenerror test. (Implementation preceded spec changes.) BUG=402376,672436 Committed: https://crrev.com/9eef72d72ab21ad9d7513a3bc6a649fd3cc1d685 Cr-Commit-Position: refs/heads/master@{#437283} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9eef72d72ab21ad9d7513a3bc6a649fd3cc1d685 Cr-Commit-Position: refs/heads/master@{#437283}
Message was sent while issue was closed.
On 2016/12/08 17:41:57, jeffcarp wrote: > On 2016/12/08 at 17:40:48, jeffcarp wrote: > > lgtm > > Just to confirm, you expect only these files to be exported from this commit, > right: > > third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/document-exit-fullscreen-timing-manual.html > third_party/WebKit/LayoutTests/imported/wpt/fullscreen/api/element-request-fullscreen-timing-manual.html Yes, exactly :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
