Bug 72173 - Pause the media element for exiting TextTrackCues when pauseOnExit is set
Summary: Pause the media element for exiting TextTrackCues when pauseOnExit is set
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 43668
  Show dependency treegraph
 
Reported: 2011-11-11 13:25 PST by Anna Cavender
Modified: 2012-02-21 15:27 PST (History)
4 users (show)

See Also:


Attachments
Code for supporting TextTrackCue pause-on-exit (8.44 KB, patch)
2012-02-20 13:27 PST, Victor Carbune
no flags Details | Formatted Diff | Diff
Fixed patch (8.60 KB, patch)
2012-02-21 04:12 PST, Victor Carbune
no flags Details | Formatted Diff | Diff
Patch with cached size of vectors (11.89 KB, patch)
2012-02-21 10:38 PST, Victor Carbune
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anna Cavender 2011-11-11 13:25:09 PST
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
Comment 1 Radar WebKit Bug Importer 2011-11-17 12:37:55 PST
<rdar://problem/10464464>
Comment 2 Victor Carbune 2012-02-20 13:27:22 PST
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 3 Eric Carlson 2012-02-20 13:55:19 PST
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.
Comment 4 Victor Carbune 2012-02-21 04:12:20 PST
Created attachment 127952 [details]
Fixed patch
Comment 5 Victor Carbune 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.
Comment 6 Eric Carlson 2012-02-21 07:33:57 PST
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.
Comment 7 Eric Carlson 2012-02-21 07:37:16 PST
(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.
Comment 8 Victor Carbune 2012-02-21 10:38:44 PST
Created attachment 127994 [details]
Patch with cached size of vectors
Comment 9 WebKit Review Bot 2012-02-21 15:27:48 PST
Comment on attachment 127994 [details]
Patch with cached size of vectors

Clearing flags on attachment: 127994

Committed r108406: <http://trac.webkit.org/changeset/108406>
Comment 10 WebKit Review Bot 2012-02-21 15:27:54 PST
All reviewed patches have been landed.  Closing bug.