[Webkit-unassigned] [Bug 72173] Pause the media element for exiting TextTrackCues when pauseOnExit is set

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72173





--- Comment #5 from Victor Carbune <vcarbune at adobe.com>  2012-02-21 05:03:33 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.