Closed Bug 891381 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: reyre, Assigned: reyre)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 1 obsolete file)

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
QA Contact: rick.eyre
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.
Assignee: nobody → rick.eyre
Attachment #8393531 - Flags: review?(cpearce)
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.
Attachment #8393531 - Flags: review?(cpearce) → review-
Thanks for review Chris.

That makes a lot of sense. I've updated TextTrack::Id to function as you described.
Attachment #8393531 - Attachment is obsolete: true
Attachment #8394812 - Flags: review?(cpearce)
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!
Attachment #8394812 - Flags: review?(cpearce) → review+
Thanks for review Chris.

Try looks green: https://tbpl.mozilla.org/?tree=Try&rev=fb3a16babdde
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a5dbde499c44
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.