[webkit-reviews] review denied: [Bug 33671] [GTK] media/video-loop.html fails intermittently on Gtk Bots : [Attachment 46571] refactored test

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 33671: [GTK] media/video-loop.html fails intermittently on Gtk Bots
https://bugs.webkit.org/show_bug.cgi?id=33671

Attachment 46571: refactored test
https://bugs.webkit.org/attachment.cgi?id=46571&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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-