Bug 72545

Summary: TextTrackList not sorted correctly
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: annacc, dglazkov, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 43668    
Attachments:
Description Flags
Proposed patch none

Description Eric Carlson 2011-11-16 14:07:01 PST
The spec mandates the sort order of the text tracks in a TextTrackList:

A media element can have a group of associated text tracks, known as the media element's list of text tracks. The text tracks are sorted as follows:

1. The text tracks corresponding to track element children of the media element, in tree order.
2. Any text tracks added using the addTextTrack() method, in the order they were added, oldest first.
3. Any media-resource-specific text tracks (text tracks corresponding to data in the media resource), in the order defined by the media resource's format specification.
Comment 1 Radar WebKit Bug Importer 2011-11-16 14:07:32 PST
<rdar://problem/10457840>
Comment 2 Eric Carlson 2011-11-16 14:33:11 PST
Created attachment 115451 [details]
Proposed patch
Comment 3 WebKit Review Bot 2011-11-16 16:10:56 PST
Comment on attachment 115451 [details]
Proposed patch

Attachment 115451 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10483897
Comment 4 Darin Adler 2011-11-16 16:48:50 PST
Comment on attachment 115451 [details]
Proposed patch

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

> Source/WebCore/html/LoadableTextTrack.cpp:128
> +    for (Node* node = m_trackElement->parentNode()->firstChild(); node; node = node->nextSibling()) {

What guarantees the track element has a non-zero parent?

> Source/WebCore/html/LoadableTextTrack.cpp:131
> +        if (static_cast<HTMLTrackElement*>(node) == m_trackElement)

This cast shouldn’t be needed because m_trackElement should automatically convert to a Node*.
Comment 5 Eric Carlson 2011-11-17 06:55:06 PST
(In reply to comment #4)
> (From update of attachment 115451 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115451&action=review
> 
> > Source/WebCore/html/LoadableTextTrack.cpp:128
> > +    for (Node* node = m_trackElement->parentNode()->firstChild(); node; node = node->nextSibling()) {
> 
> What guarantees the track element has a non-zero parent?
> 
That should not happen because a track element are only added to the list when insertedIntoTree is called, and it is always removed from the list when willRemove is called, but I added an ASSERT.

> > Source/WebCore/html/LoadableTextTrack.cpp:131
> > +        if (static_cast<HTMLTrackElement*>(node) == m_trackElement)
> 
> This cast shouldn’t be needed because m_trackElement should automatically convert to a Node*.

Thanks.
Comment 6 Eric Carlson 2011-11-17 06:55:35 PST
http://trac.webkit.org/changeset/100616
Comment 7 Eric Carlson 2011-11-17 06:56:11 PST
Comment on attachment 115451 [details]
Proposed patch

Clearing flags, changes committed.