At least on GTK skipping: media/video-load-require-user-gesture.html media/video-play-require-user-gesture.html works around the issue but a proper solution is required. I won't have time to work on it this week though. Skipping those two for now.
The flaky tests list: Unexpected flakiness: text diff mismatch (25) fast/forms/text-input-event.html = TEXT PASS media/track/track-mode.html = TEXT PASS media/video-loop.html = TEXT PASS media/video-muted.html = TEXT PASS media/video-pause-immediately.html = TEXT PASS media/video-play-empty-events.html = TEXT PASS media/video-play-pause-events.html = TEXT PASS media/video-play-pause-exception.html = TEXT PASS media/video-playbackrate.html = TEXT PASS media/video-played-collapse.html = TEXT PASS media/video-played-ranges-1.html = TEXT PASS media/video-played-reset.html = TEXT PASS media/video-preload.html = TEXT PASS media/video-reverse-play-duration.html = TEXT PASS media/video-seek-past-end-paused.html = TEXT PASS media/video-seek-past-end-playing.html = TEXT PASS media/video-set-rate-from-pause.html = TEXT PASS media/video-source-load.html = TEXT PASS media/video-src-invalid-poster.html = TEXT PASS media/video-src-none.html = TEXT PASS media/video-timeupdate-during-playback.html = TEXT PASS media/video-timeupdate-reverse-play.html = TEXT PASS media/video-transformed.html = TEXT PASS media/video-volume.html = TEXT PASS media/video-zoom-controls.html = TEXT PASS
Running the tests with --run-singly (so each test uses its own DumpRenderTree) works fine. That reinforces the idea that the MediaPlaybackRequiresUserGesture property, set on the following tests: media/video-load-require-user-gesture.html media/video-play-require-user-gesture.html is affecting the others.
Created attachment 137687 [details] Patch
Comment on attachment 137687 [details] Patch The API looks good to me. We just need one more GTK+ reviwer to approve the API and give the official r+.
Comment on attachment 137687 [details] Patch Thanks!
Comment on attachment 137687 [details] Patch Clearing flags on attachment: 137687 Committed r114506: <http://trac.webkit.org/changeset/114506>
All reviewed patches have been landed. Closing bug.
Comment on attachment 137687 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137687&action=review > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:430 > + "media-playback-requires-user-gesture", FALSE, This shouldn't be necessary because both tests *should* call window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false) before ending. Also, I don't believe that any of the other DRT implementations does this cleanup. Does the problem reproduce without this?
Reopening because the patch was rolled out.
(In reply to comment #8) > (From update of attachment 137687 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137687&action=review > > > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:430 > > + "media-playback-requires-user-gesture", FALSE, > > This shouldn't be necessary because both tests *should* call window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false) before ending. Also, I don't believe that any of the other DRT implementations does this cleanup. > Right. > Does the problem reproduce without this? No, but I don't think it's an issue to keep this line anyway. I've been trying Simon's patch, after some investigation I think the tests are actually badly calling the InternalSettings API :) Idl file has: void setMediaPlaybackRequiresUserGesture(in boolean enabled) raises(DOMException); And the tests do: window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false); Fixing the tests fixes flakyness here. But I think we still need Simon's patch anyway, maybe in a new bugzilla entry Simon?
Created attachment 137788 [details] Patch
(In reply to comment #10) > > I've been trying Simon's patch, after some investigation I think the tests are actually badly calling the InternalSettings API :) > > Idl file has: void setMediaPlaybackRequiresUserGesture(in boolean enabled) raises(DOMException); > > And the tests do: window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false); > Oops :-(
Comment on attachment 137788 [details] Patch Thanks for fixing my mistake!
Comment on attachment 137788 [details] Patch Clearing flags on attachment: 137788 Committed r114586: <http://trac.webkit.org/changeset/114586>
(In reply to comment #10) > (In reply to comment #8) > > (From update of attachment 137687 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=137687&action=review > > > > > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:430 > > > + "media-playback-requires-user-gesture", FALSE, > > > > This shouldn't be necessary because both tests *should* call window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false) before ending. Also, I don't believe that any of the other DRT implementations does this cleanup. > > > > Right. > > > Does the problem reproduce without this? > > No, but I don't think it's an issue to keep this line anyway. When I added that line to the DumpRenderTree, I was assuming that, under some concurrency situations, some tests were run with that setting enabled. Since they passed fine when I used the --run-singly option, I though I was right. However, after Phil's findings: > I've been trying Simon's patch, after some investigation I think the tests are actually badly calling the InternalSettings API :) > > Idl file has: void setMediaPlaybackRequiresUserGesture(in boolean enabled) raises(DOMException); > > And the tests do: window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false); This does seem the cause for flakyness, and not the other. > Fixing the tests fixes flakyness here. But I think we still need Simon's patch anyway, maybe in a new bugzilla entry Simon? Yes, I guess we should still provide the API to enable/disable the mediaPlaybackRequiresUserGesture setting (it is provided in other ports), but we no longer need to clean up that setting from the DumpRenderTree. I'll file a new bug for that, and attaching this patch without the DumpRenderTree part. Thanks for the investigation, and sorry for the trouble!