Bug 72173

<rdar://problem/10464464>

Created attachment 127844 [details] Code for supporting TextTrackCue pause-on-exit Pause-on-exit flag support and a minor improvement for missed cues processing

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.

Created attachment 127952 [details] Fixed patch

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

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.

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

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

Comment on attachment 127994 [details] Patch with cached size of vectors Clearing flags on attachment: 127994 Committed r108406: <http://trac.webkit.org/changeset/108406>

All reviewed patches have been landed. Closing bug.