[webkit-reviews] review denied: [Bug 60972] Audio element doesn't emit the 'playing' event every time it starts playing, after it has finished playing : [Attachment 108631] updated patch

[bugzilla-daemon at webkit.org]

Philippe Normand <pnormand at igalia.com> has denied Arun Patole
<bmf834 at motorola.com>'s request for review:
Bug 60972: Audio element doesn't emit the 'playing' event every time it starts
playing, after it has finished playing
https://bugs.webkit.org/show_bug.cgi?id=60972

Attachment 108631: updated patch
https://bugs.webkit.org/attachment.cgi?id=108631&action=review

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


Looks good in general, just some nits and a speed improvement suggestion for
the test.

> Source/WebCore/ChangeLog:8
> +	   As per the latest spec, paused attribute should be set to true and
fire 'pause' event on end of media playback.

"latest" is vague, if you can't mention the spec revision that introduced this
change.
For the rest of the sentence here's my take :)
the paused attribute should be set to true and the media element should emit a
'paused' at the end of playback.

> Source/WebCore/ChangeLog:13
> +	   (WebCore::HTMLMediaElement::mediaPlayerTimeChanged): set m_paused to
true and schedule 'pause' event if end of playback.

s/if end of playback/when playback ended/

> LayoutTests/ChangeLog:8
> +	   As per the latest spec, paused attribute should be set to true and
fire 'pause' event on end of media playback.

No need to copy/paste from the other ChangeLog entry.

> LayoutTests/ChangeLog:14
> +	   * media/event-attributes-expected.txt: Modified as the 'pause' event
is fired on end of media playback.
> +	   * media/event-attributes.html: Modified as the 'pause' event is
fired on end of media playback.
> +	   * media/media-element-play-after-eos-expected.txt: Added.
> +	   * media/media-element-play-after-eos.html: Added.
> +	   * media/video-loop-expected.txt: Modified as the 'pause' event is
fired on end of media playback.

Instead of copy/pasting 3 times the same sentence let's just put it as a
general note above.

> LayoutTests/media/media-element-play-after-eos.html:12
> +		   run("mediaElement.src = findMediaFile('audio',
'content/silence')");

We usually don't log this, no need for run()

> LayoutTests/media/media-element-play-after-eos.html:32
> +		    run("mediaElement.play()");
> +		    repeated = true;

So the test plays the silence file twice? It's about 1.1 seconds so this test
runs in more than 2 seconds. Maybe it'd be worth to perform a seek near the end
of the media when the "playing" event is received.