Pause the media element for exiting TextTrackCues when pauseOnExit is set

[Anna Cavender]

4.8.10.8 Playing the media resource: http://www.whatwg.org/specs/web-apps/current-work/#playing-the-media-resource

Specifically, "When the current playback position of a media element changes..." Step 7.

For more info on the pause-on-exit flag, see http://www.whatwg.org/specs/web-apps/current-work/#text-track-cue-pause-on-exit-flag

[Radar WebKit Bug Importer]

<rdar://problem/10464464>

[Victor Carbune]

Created attachment 127844 [details]
Code for supporting TextTrackCue pause-on-exit

Pause-on-exit flag support and a minor improvement for missed cues processing

[Eric Carlson]

Comment on attachment 127844 [details]
Code for supporting TextTrackCue pause-on-exit

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.

[Victor Carbune]

Created attachment 127952 [details]
Fixed patch

[Victor Carbune]

(In reply to comment #3)
> (From update of attachment 127844 [details])
> 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()?

I'm just worried about readability - maybe I should change the function call everywhere within the method? But isn't the Vector::size() function call itself cheap? It seems to be just a const method returning a member variable.

> > 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.

Indeed. Changed it to simple-captions.vtt.

> > 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())
Done.

> > LayoutTests/media/track/track-cues-pause-on-exit.html:13
> > +            var currentCueNo = 0;
> 
> Nit: I don't think the abbreviation of "Number" is helpful.
Done.

[Eric Carlson]

Comment on attachment 127952 [details]
Fixed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127952&action=review

> LayoutTests/ChangeLog:10
> +        * media/track/captions-webvtt/pause-on-exit.vtt: Added.

You forgot to update after changing the file name.

[Eric Carlson]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 127844 [details] [details])
> > 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()?
> 
> I'm just worried about readability - maybe I should change the function call everywhere within the method? But isn't the Vector::size() function call itself cheap? It seems to be just a const method returning a member variable.

I see now that most of the loops in updateActiveTextTrackCues don't cache the vector size before the loop. One way or the other, we should try to be consistent.

[Victor Carbune]

Created attachment 127994 [details]
Patch with cached size of vectors

[WebKit Review Bot]

Comment on attachment 127994 [details]
Patch with cached size of vectors

Clearing flags on attachment: 127994

Committed r108406: <http://trac.webkit.org/changeset/108406>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.