Bug 123825 – Add "id" attribute to TextTrack

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Brendan Long]

Add "id" attribute to TextTrack

[Brendan Long]

Created attachment 216070 [details]
Patch

This adds "id" to the API. I figured I'd do this patch separately from implementing the ID in GStreamer.

[Brendan Long]

Oh, and the reason TextTrack didn't have an "id" before was that it wasn't in the spec, but it is now:

http://www.w3.org/TR/html5/embedded-content-0.html#dom-texttrack-id

[Eric Carlson]

Comment on attachment 216070 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216070&action=review

> Source/WebCore/html/track/InbandTextTrack.cpp:62
>  InbandTextTrack::InbandTextTrack(ScriptExecutionContext* context, TextTrackClient* client, PassRefPtr<InbandTextTrackPrivate> tracksPrivate)
> -    : TextTrack(context, client, emptyString(), tracksPrivate->label(), tracksPrivate->language(), InBand)
> +    : TextTrack(context, client, emptyString(), tracksPrivate->id(), tracksPrivate->label(), tracksPrivate->language(), InBand)
>      , m_private(tracksPrivate)

Nit: while you are here, can you please change "tracksPrivate" to "trackPrivate" since there is only one private track?

> Source/WebCore/html/track/LoadableTextTrack.cpp:40
>  LoadableTextTrack::LoadableTextTrack(HTMLTrackElement* track, const String& kind, const String& label, const String& language)
> -    : TextTrack(&track->document(), track, kind, label, language, TrackElement)
> +    : TextTrack(&track->document(), track, kind, emptyString(), label, language, TrackElement)

Doesn't LoadableTextTrack need to override "id()" so it can return the HTMLTrackElement id attribute? From the spec:

For tracks that correspond to track elements, the track's identifier is the value of the element's id attribute, if any

> Source/WebCore/html/track/TrackBase.h:50
> +    AtomicString id() const { return m_id; }

This should be virtual, see above.

[Brendan Long]

(In reply to comment #3)
> For tracks that correspond to track elements, the track's identifier is the value of the element's id attribute, if any
> 
> > Source/WebCore/html/track/TrackBase.h:50
> > +    AtomicString id() const { return m_id; }
> 
> This should be virtual, see above.

Oh, somehow I missed that. I'll add it.

[Brendan Long]

Created attachment 216083 [details]
Patch

I fixed tracksPrivate, made LoadableTextTrack use m_trackElement->id, and added a test using out-of-band tracks.

[EFL EWS Bot]

Comment on attachment 216083 [details]
Patch

Attachment 216083 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/21238019

[Brendan Long]

Internal compiler error eh? :\

[Brendan Long]

Should I resubmit this, or just ignore the weird EFL compiler crash?

[Brendan Long]

Created attachment 216179 [details]
Patch

I rebased this after another patch was committed.

[Build Bot]

Comment on attachment 216179 [details]
Patch

Attachment 216179 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/21948006

New failing tests:
media/track/track-id.html

[Build Bot]

Created attachment 216182 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Brendan Long]

Created attachment 216183 [details]
Patch

Apparently I need to add an extra blank line to media tests after r158743. I'll fix the in-band tests in a different patch.

[WebKit Commit Bot]

Comment on attachment 216183 [details]
Patch

Clearing flags on attachment: 216183

Committed r158760: <http://trac.webkit.org/changeset/158760>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.