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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

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 1

[Rick Eyre (:reyre)]

Attachment: v1: TextTrack's ActiveCues should return null of TextTrack's mode is set to 'disabled' r=rillan

Comment 2

[Ralph Giles (:rillian)]

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?

Comment 3

[Rick Eyre (:reyre)]

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

Comment 4

[Rick Eyre (:reyre)]

Thanks for review Ralph.

Try: https://tbpl.mozilla.org/?tree=Try&rev=c73b9a7d1ad5

Comment 5

[Rick Eyre (:reyre)]

Looks good.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff8daab3e449

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff8daab3e449