Layout test media/context-menu-actions.html is failing since r229466 "webkitfullscreenchange event not fired at the same time as :-webkit-full-screen pseudo selector changes; causes glitchiness". I'll add a Failure expectation. --- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/media/context-menu-actions-expected.txt +++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/media/context-menu-actions-actual.txt @@ -1,3 +1,4 @@ +CONSOLE MESSAGE: line 122: TypeError: undefined is not an object (evaluating 'this.captionMenu.style') Test the various actions available in the HTML5 media element context-menu. @@ -21,7 +22,7 @@ EXPECTED (video.webkitDisplayingFullscreen == 'false') OK Toggling fullscreen state -EXPECTED (video.webkitDisplayingFullscreen == 'true') OK +EXPECTED (video.webkitDisplayingFullscreen == 'true'), OBSERVED 'false' FAIL END OF TEST
This test assumes that the webkitRequestFullscreen() implementation is entirely synchronous, which is most definitely isn't. Each of these tests (or at least, each that has an associated event) should be waiting on that event, and not assuming that the results will change within the current run-loop.
I'll remove the REGRESSION tag then. It doesn't seem helpful here, as the behavior change was intended.
*** Bug 121742 has been marked as a duplicate of this bug. ***
Created attachment 402005 [details] Patch
Created attachment 402012 [details] Patch
Comment on attachment 402012 [details] Patch Problem is that 50ms delays are (a) racy, and (b) going to add up when running 60,000 layout tests all at once. If there's no way to check whether the window has been fullscreened, then this is the best we can do. But I bet there is a way to check? :)
Comment on attachment 402012 [details] Patch I must agree with Michael here
(In reply to Michael Catanzaro from comment #6) > Problem is that 50ms delays are (a) racy, and (b) going to add up when > running 60,000 layout tests all at once. If there's no way to check whether > the window has been fullscreened, then this is the best we can do. But I bet > there is a way to check? :) I understand, but the ChangeLog explains why the "proper" way to check if the window has been fullscreened (onwebkitfullscreenchange) isn't working: Simply setting an event handler on video.onwebkitfullscreenchange wouldn't have been enough, since the event is triggered before the fullscreen operation has been completed and checking for video.webkitDisplayingFullscreen == true would still fail by then. The extra 50ms delay was the only solution I found. I tried to set it to the lowest possible value that doesn't trigger errors on my local system (25ms was too low, 100ms seemed a waste).
OK, in that case, maybe a hardcoded delay is unavoidable. Maybe "loop" the timeout say, 20 times, by reattaching the timeout if it's not fullscreen yet? That way we can reduce the race window and only fail the test if it's still not fullscreen after one second, without slowing down the test by more than 50ms more than required. I'm sure 50ms is enough for a typical laptop or desktop, but imagine the test is running in a container on a VM with limited resources on a host system that's doing 200 other things at the same time... or imagine the test passes when the system is doing 100 other things at the same time rather than 200... won't be fun to debug that.
(In reply to Michael Catanzaro from comment #9) > Maybe "loop" the timeout say, 20 times, by reattaching the timeout if it's > not fullscreen yet? That way we can reduce the race window and only fail the > test if it's still not fullscreen after one second That's a good suggestion, thanks!
Comment on attachment 402012 [details] Patch There seems to be a beginfullscreen event. I think we should use this instead of just a timeout
(In reply to Xabier Rodríguez Calvar from comment #11) > There seems to be a beginfullscreen event. I think we should use this > instead of just a timeout Neither that webkitbeginfullscreen event[1] nor its couterpart webkitendfullscreenevent is ever triggered when using FULLSCREEN_API[2], so I don't think they're an option unless we change (and potentially break) the HTMLMediaElement code. I suspect that some sort of wait is needed after all, since that's how other tests deal with fullscreen changes. For instance, helper functions in media-fullscreen.js call testExpectedEventually()[3][4][5], which is implemented as a sleep-and-retry loop[6]. [1] https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/HTMLMediaElement.cpp#L5974 [2] https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/HTMLMediaElement.cpp#L5953 [3] https://github.com/WebKit/webkit/blob/master/LayoutTests/media/media-fullscreen.js#L138 [4] https://github.com/WebKit/webkit/blob/master/LayoutTests/media/media-fullscreen.js#L36 [5] https://github.com/WebKit/webkit/blob/master/LayoutTests/media/media-fullscreen.js#L43 [6] https://github.com/WebKit/webkit/blob/master/LayoutTests/media/video-test.js#L119
(In reply to Enrique Ocaña from comment #12) > Neither that webkitbeginfullscreen event[1] nor its couterpart > webkitendfullscreenevent is ever triggered when using FULLSCREEN_API[2], so > I don't think they're an option unless we change (and potentially break) the > HTMLMediaElement code. Sigh. Go for the loop Michael suggested then.
Created attachment 402211 [details] Patch
Comment on attachment 402211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402211&action=review r+ but you need to fix the changelog. Then I have a couple of questions. First is if this patch breaks any other tests and need a a rebase line. The seconds would be why is the test still marked as crash. > LayoutTests/ChangeLog:19 > + would be lost) in case we wait forever for the comparison to evaluato to the expected value. evaluate
(In reply to Xabier Rodríguez Calvar from comment #15) > Then I have a couple of questions. First is if this patch breaks any other tests and need a a rebase line. Indeed, some tests appear to be broken. I'll need to have a look. > The seconds would be why is the test still marked as crash. The crash issue belongs to bug 198830 and seems to be a GLX related problem out of the scope of bug 183611. I wanted to focus on one thing at a time instead of tackling two unrelated problems.
Created attachment 403297 [details] Patch
Committed r263835: <https://trac.webkit.org/changeset/263835> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403297 [details].
<rdar://problem/65023862>
Comment on attachment 403297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403297&action=review > LayoutTests/platform/gtk/TestExpectations:3964 > -webkit.org/b/183611 webkit.org/b/198830 media/context-menu-actions.html [ Failure Crash ] > +webkit.org/b/198830 media/context-menu-actions.html [ Crash ] So now this test is no longer flaky on GTK? Was this an intended change? Is this expected to always crash now?
Comment on attachment 403297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403297&action=review >> LayoutTests/platform/gtk/TestExpectations:3964 >> +webkit.org/b/198830 media/context-menu-actions.html [ Crash ] > > So now this test is no longer flaky on GTK? Was this an intended change? Is this expected to always crash now? I fixed the flakiness (the test no longer fails), but the crash still can happen sometimes (not always, we could call it a flaky crash) and is tracked on bug 198830, out of the scope of this bug.
Comment on attachment 403297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403297&action=review >>> LayoutTests/platform/gtk/TestExpectations:3964 >>> +webkit.org/b/198830 media/context-menu-actions.html [ Crash ] >> >> So now this test is no longer flaky on GTK? Was this an intended change? Is this expected to always crash now? > > I fixed the flakiness (the test no longer fails), but the crash still can happen sometimes (not always, we could call it a flaky crash) and is tracked on bug 198830, out of the scope of this bug. OK so the new expectation should be [ Pass Crash ] I think...
(In reply to Philippe Normand from comment #22) > OK so the new expectation should be [ Pass Crash ] I think... Yup.
For reference, I've corrected the TestExpectations line in bug 213877.