891381 - [webvtt] Ensure that TextTrack::Id is being loaded correctly

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

There are different rules for where the TextTrack's Id should be loaded from depending on the source of the TextTrack i.e. did it come from a TrackElement or a MediaResource, etc?

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-texttrack-id

Comment 1

[Rick Eyre (:reyre)]

The only one we can really account for right now is tracks that come from a track element since we don't have any in band tracks from media resources yet.

Comment 2

[Rick Eyre (:reyre)]

Attachment: v1: Tracks loaded via TrackElement should have the same id as the TrackElement r=cpearce

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8393531 [details] [diff] [review]
v1: Tracks loaded via TrackElement should have the same id as the TrackElement r=cpearce

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

I think we can do better. Here we are making multiple copies of some state (the track id). This is bad, because the state can get out of sync if we make a logic error sometime. This has been a source of bugs in our code in the past. ;)

So, I think we should have TextTrack::GetId() do the Right Thing, instead of having multiple places in the code that need to remember to update TextTrack::mId when required. So please change TextTrack::GetId() to instead retrieve the id from its parent HTMLTrackElement every time it's called. This means if the HTMLTrackElement's id changes, the TextTrack's id will automatically reflect that. That's the behaviour we want right? Currently the TextTrack's id should always be the HTMLTrackElement's right?

If/when we add support for in-band text tracks, we'll need to change GetId() to reflect that.

Comment 4

[Rick Eyre (:reyre)]

Attachment: v2: Tracks loaded via TrackElement should have the same id as the TrackElement r=cpearce

Thanks for review Chris.

That makes a lot of sense. I've updated TextTrack::Id to function as you described.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8394812 [details] [diff] [review]
v2: Tracks loaded via TrackElement should have the same id as the TrackElement r=cpearce

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

Good!

Comment 6

[Rick Eyre (:reyre)]

Thanks for review Chris.

Try looks green: https://tbpl.mozilla.org/?tree=Try&rev=fb3a16babdde

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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