Bug 123825 - Add "id" attribute to TextTrack
Summary: Add "id" attribute to TextTrack
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brendan Long
URL:
Keywords:
Depends on:
Blocks: 85035
  Show dependency treegraph
 
Reported: 2013-11-05 13:27 PST by Brendan Long
Modified: 2013-11-06 10:51 PST (History)
11 users (show)

See Also:


Attachments
Patch (16.37 KB, patch)
2013-11-05 13:32 PST, Brendan Long
no flags Details | Formatted Diff | Diff
Patch (19.60 KB, patch)
2013-11-05 14:49 PST, Brendan Long
no flags Details | Formatted Diff | Diff
Patch (19.55 KB, patch)
2013-11-06 08:31 PST, Brendan Long
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (495.87 KB, application/zip)
2013-11-06 09:04 PST, Build Bot
no flags Details
Patch (19.55 KB, patch)
2013-11-06 09:05 PST, Brendan Long
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brendan Long 2013-11-05 13:27:56 PST
Add "id" attribute to TextTrack
Comment 1 Brendan Long 2013-11-05 13:32:47 PST
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.
Comment 2 Brendan Long 2013-11-05 13:33:32 PST
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
Comment 3 Eric Carlson 2013-11-05 14:04:54 PST
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.
Comment 4 Brendan Long 2013-11-05 14:19:00 PST
(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.
Comment 5 Brendan Long 2013-11-05 14:49:18 PST
Created attachment 216083 [details]
Patch

I fixed tracksPrivate, made LoadableTextTrack use m_trackElement->id, and added a test using out-of-band tracks.
Comment 6 EFL EWS Bot 2013-11-05 15:07:32 PST
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
Comment 7 Brendan Long 2013-11-05 15:11:32 PST
Internal compiler error eh? :\
Comment 8 Brendan Long 2013-11-05 16:27:55 PST
Should I resubmit this, or just ignore the weird EFL compiler crash?
Comment 9 Brendan Long 2013-11-06 08:31:21 PST
Created attachment 216179 [details]
Patch

I rebased this after another patch was committed.
Comment 10 Build Bot 2013-11-06 09:04:43 PST
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
Comment 11 Build Bot 2013-11-06 09:04:45 PST
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
Comment 12 Brendan Long 2013-11-06 09:05:57 PST
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.
Comment 13 WebKit Commit Bot 2013-11-06 10:51:20 PST
Comment on attachment 216183 [details]
Patch

Clearing flags on attachment: 216183

Committed r158760: <http://trac.webkit.org/changeset/158760>
Comment 14 WebKit Commit Bot 2013-11-06 10:51:23 PST
All reviewed patches have been landed.  Closing bug.