[webkit-reviews] review granted: [Bug 72173] Pause the media element for exiting TextTrackCues when pauseOnExit is set : [Attachment 127844] Code for supporting TextTrackCue pause-on-exit

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Victor Carbune
<vcarbune at adobe.com>'s request for review:
Bug 72173: Pause the media element for exiting TextTrackCues when pauseOnExit
is set
https://bugs.webkit.org/show_bug.cgi?id=72173

Attachment 127844: Code for supporting TextTrackCue pause-on-exit
https://bugs.webkit.org/attachment.cgi?id=127844&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=127844&action=review


> Source/WebCore/html/HTMLMediaElement.cpp:1061
> +    for (size_t i = 0; !m_paused && i < previousCues.size(); ++i) {

Nit: is there any reason to not use previousCuesSize instead of
previousCues.size()?

> LayoutTests/media/track/captions-webvtt/pause-on-exit.vtt:17
> +WEBVTT
> +
> +0
> +00:00:05.000 --> 00:00:05.200
> +First cue
> +
> +1
> +00:00:05.210 --> 00:00:05.400
> +Lorem
> +
> +2
> +00:00:05.410 --> 00:00:05.600
> +ipsum
> +
> +3
> +00:00:05.610 --> 00:00:05.700
> +dolor

Nit: I think the name of this file is misleading, it doesn't necessarily have
anything to do with pause-on-exit.

> LayoutTests/media/track/track-cues-pause-on-exit-expected.txt:15
> +RUN(video.play())
> +EXPECTED (video.paused == 'false') OK
> +EXPECTED (currentCue.id == '0') OK
> +EXPECTED (video.paused == 'true') OK
> +RUN(video.play())
> +EXPECTED (currentCue.id == '1') OK
> +EXPECTED (video.paused == 'false') OK
> +EXPECTED (currentCue.id == '2') OK
> +EXPECTED (video.paused == 'true') OK
> +RUN(video.play())
> +EXPECTED (currentCue.id == '3') OK

Nit: I try to make it possible to understand how a test works just from the
results. I think these results would be easier to understand if you log the
'exit' event and put a blank line between test steps: 

RUN(video.play())
EXPECTED (video.paused == 'false') OK

EVENT(exit)
EXPECTED (currentCue.id == '0') OK
EXPECTED (video.paused == 'true') OK
RUN(video.play())

EVENT(exit)
EXPECTED (currentCue.id == '1') OK
EXPECTED (video.paused == 'false') OK

EVENT(exit)
EXPECTED (currentCue.id == '2') OK
EXPECTED (video.paused == 'true') OK
RUN(video.play())

> LayoutTests/media/track/track-cues-pause-on-exit.html:13
> +	       var currentCueNo = 0;

Nit: I don't think the abbreviation of "Number" is helpful.