Closed Bug 969506 Opened 6 years ago Closed 6 years ago

[webvtt] TextTrack's ActiveCues should return null of TextTrack's mode is set to 'disabled'

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: reyre, Assigned: reyre)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

We were doing this, but a slight refactor from bug 865407 regressed it. I've added a test to make sure that doesn't happen again.
Comment on attachment 8372443 [details] [diff] [review]
v1: TextTrack's ActiveCues should return null of TextTrack's mode is set to 'disabled' r=rillan

Review of attachment 8372443 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/TextTrack.cpp
@@ +147,5 @@
>  
>  void
>  TextTrack::UpdateActiveCueList()
>  {
> +  if (!mTextTrackList) {

This means we still do the work of updating the cue list even when the track is disabled. Is that also correct?
Attachment #8372443 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) from comment #2)
> Comment on attachment 8372443 [details] [diff] [review]
> v1: TextTrack's ActiveCues should return null of TextTrack's mode is set to
> 'disabled' r=rillan
> 
> Review of attachment 8372443 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/TextTrack.cpp
> @@ +147,5 @@
> >  
> >  void
> >  TextTrack::UpdateActiveCueList()
> >  {
> > +  if (!mTextTrackList) {
> 
> This means we still do the work of updating the cue list even when the track
> is disabled. Is that also correct?

Not necessarily. Someone could call UpdateActiveCueList when Mode is disabled and it would update, but the only current callers are GetActiveCueArray and GetActiveCues, which is what the update method on TextTrackManager is calling. I put the check for whether Mode is disabled in those two functions because whether or not Mode is disabled they'll return something or not.
Thanks for review Ralph.

Try: https://tbpl.mozilla.org/?tree=Try&rev=c73b9a7d1ad5
Looks good.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ff8daab3e449
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.