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

RESOLVED FIXED in mozilla31

Status

()

Core
Audio/Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: reyre, Assigned: reyre)

Tracking

(Blocks: 1 bug)

unspecified
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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
(Assignee)

Updated

4 years ago
Blocks: 629350
Depends on: 882706
(Assignee)

Updated

4 years ago
QA Contact: rick.eyre
(Assignee)

Comment 1

4 years ago
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)

Comment 2

4 years ago
Created attachment 8393531 [details] [diff] [review]
v1: Tracks loaded via TrackElement should have the same id as the TrackElement r=cpearce
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-
(Assignee)

Comment 4

4 years ago
Created attachment 8394812 [details] [diff] [review]
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.
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+
(Assignee)

Comment 6

4 years ago
Thanks for review Chris.

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