That test does a seek() and fires up a timer. In our player the pipeline can take time to complete the seek and in some cases the timer will be fired too early. That test should be rewritten to rely on timeupdate events instead of a timer, in my opinion.
Created attachment 46571 [details] refactored test
(In reply to comment #1) > Created an attachment (id=46571) [details] > refactored test This is a proposed solution, can it be tested on mac as well? I had to change a line of the expected result and I am not totally sure it is correct.
*** Bug 33497 has been marked as a duplicate of this bug. ***
It seems that either r53277 or 53281 caused this to start failing consistently. It's been failing for the last 8 builds on the Gtk builder.
Never mind, it failed 7 out of 8 builds. It did not fail in http://build.webkit.org/results/GTK%20Linux%20Release/r53283%20(7663)/results.html
I preffer to have Eric Carlson or someone else with more knowledge than me reviewing this patch. I'll skip the test while you get this one reviewed, please unskip when committing! =)
Comment on attachment 46571 [details] refactored test This patch has several problems: > else if (timeupdateEventCount >= 10) > This magic number seems fragile, if a media engine doesn't fire 10 'timeupdate' events (because of processor load or whatever) it will fail. > testExpected("mediaElement.currentTime", (video.duration - 0.4).toFixed(2), '<'); > This will log the movie's duration, which is not exactly the on every platform because of the different movie files used. > if (timeupdateEventCount == 2) { > // make sure we are playing, seek to near the end so > Three space tabs? > function ended() { > Brace should be on the next line. In general, replacing a timer with a 'timeupdate' event handler seems wrong, as that event is extremely platform and machine dependent. I would rather see this test rewritten in a way that removes as much indeterminism as possible. I would also like to see revised tests cleaned up so they have reasonable looking markup. You didn't make this any worse, but the lack of a body, the stray "</head>", etc should be cleaned up as long as the file is being changed. marking r-
Why don't I revise this test and reattach so you can test on a GTK machine to make sure my changes fix the intermittent failures.
(In reply to comment #8) > Why don't I revise this test and reattach so you can test on a GTK machine to > make sure my changes fix the intermittent failures. Ok, you will be faster than me at reworking this test :) Thanks for the review anyway, I will be happy to test here the reworked test if needed.
Created attachment 47354 [details] Revised test Can you see if is more consistent?
(In reply to comment #10) > Created an attachment (id=47354) [details] > Revised test > > Can you see if is more consistent? I was unable to make it fail :)
Created attachment 47777 [details] Proposed patch
Attachment 47777 [details] was posted by a committer and has review+, assigning to Eric Carlson for commit.
Comment on attachment 47777 [details] Proposed patch Clearing the r+ flag for now because I need to generate new results but won't be able to get to it for a bit.
Created attachment 64860 [details] patch Revised slightly to pause before seeking to try to avoid looping before the 'seeked' event fires on really slow machines.
Could you please take the opportunity to unskip the test, too? =)
trac.webkit.org/changeset/65684
(In reply to comment #16) > Could you please take the opportunity to unskip the test, too? =) Darn, missed your comment.
(In reply to comment #18) > (In reply to comment #16) > > Could you please take the opportunity to unskip the test, too? =) > > Darn, missed your comment. Unskipped in http://trac.webkit.org/changeset/66487