[webkit-reviews] review denied: [Bug 68589] [Qt][Gtk] Wrong state when pausing a video in the "playing" event handler : [Attachment 108344] Patch.

[bugzilla-daemon at webkit.org]

Philippe Normand <pnormand at igalia.com> has denied Yael
<yael.aharon at nokia.com>'s request for review:
Bug 68589: [Qt][Gtk] Wrong state when pausing a video in the "playing" event
handler
https://bugs.webkit.org/show_bug.cgi?id=68589

Attachment 108344: Patch.
https://bugs.webkit.org/attachment.cgi?id=108344&action=review

------- Additional Comments from Philippe Normand <pnormand at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=108344&action=review


Thanks for the test, just some nits to fix.

> LayoutTests/media/video-playing-and-pause.html:7
> +	       var timeAfterPlay;

This variable is not used, not needed :)

> LayoutTests/media/video-playing-and-pause.html:13
> +		   v.src="content/counting.ogv";

findMediaElement();
video.src = findMediaFile("video", "content/test");

doesn't work?

> LayoutTests/media/video-playing-and-pause.html:26
> +		   var v = document.getElementById("v");

This is not needed. findMediaElement(), if called before, will set a global
video variable.

> LayoutTests/media/video-playing-and-pause.html:27
> +		   v.pause();

video.pause() should work

> LayoutTests/media/video-playing-and-pause.html:35
> +		       document.getElementById("parentDiv").innerHTML = "FAIL";


You need endTest() after this statement, I think. Otherwise the test might just
time out.

> LayoutTests/media/video-playing-and-pause.html:47
> +	   <p>Test that pausing the media element in "playing" event handler
pauses the media immediately.</p>

Maybe worth addind a comment about the result showing the first frame, I leave
that up to you.

> LayoutTests/media/video-playing-and-pause.html:48
> +	   <video id="v" autoplay controls></video>

No need for an id.