HTMLMediaElement's .textTracks property does not reflect <track> element

[Antoine Quint]

We fail the Opera-submitted test at http://w3c-test.org/html/tests/submission/Opera/media/interfaces/TextTrackList/getter.html.

The first test fails because we don't return the <track> element added to the <video> element as one of the text tracks. Note that when we do, we should respect the first rule of the spec for ordering of text tracks which says that the first items of the list are <track> elements (see http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#list-of-text-tracks).

[Radar WebKit Bug Importer]

<rdar://problem/12758151>

[Eric Carlson]

(In reply to comment #0)
> We fail the Opera-submitted test at http://w3c-test.org/html/tests/submission/Opera/media/interfaces/TextTrackList/getter.html.
> 
> The first test fails because we don't return the <track> element added to the <video> element as one of the text tracks. 

This happens because the <video> element is not in the document when the track was added, so HTMLTrackElement::insertedInto does not notify the HTMLMediaElement parent. Originally the parent element was always notified, but this was changed in http://trac.webkit.org/changeset/114351. I do not know why this change was made.

> Note that when we do, we should respect the first rule of the spec for ordering of text tracks which says that the first items of the list are <track> elements (see http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#list-of-text-tracks).

We do this, see TextTrackList::item.

[Hajime Morrita]

(In reply to comment #2)
> (In reply to comment #0)
> > We fail the Opera-submitted test at http://w3c-test.org/html/tests/submission/Opera/media/interfaces/TextTrackList/getter.html.
> > 
> > The first test fails because we don't return the <track> element added to the <video> element as one of the text tracks. 
> 
> This happens because the <video> element is not in the document when the track was added, so HTMLTrackElement::insertedInto does not notify the HTMLMediaElement parent. Originally the parent element was always notified, but this was changed in http://trac.webkit.org/changeset/114351. I do not know why this change was made.

This sounds like a bug. I'll take a look.
Thanks for letting me know this.

[Hajime Morrita]

Created attachment 176410 [details]
Patch

[Dean Jackson]

Comment on attachment 176410 [details]
Patch

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

> Source/WebCore/html/HTMLTrackElement.cpp:87
>  void HTMLTrackElement::removedFrom(ContainerNode* insertionPoint)
>  {
> -    HTMLMediaElement* parent = mediaElement();
> -    if (!parent && WebCore::isMediaElement(insertionPoint))
> -        parent = toMediaElement(insertionPoint);
> -    if (parent)
> -        parent->willRemoveTrack(this);
> -
> +    if (!parentNode() && WebCore::isMediaElement(insertionPoint))
> +        toMediaElement(insertionPoint)->didRemoveTrack(this);

Just checking the logic here. Previously this was calling willRemoveTrack on the parent/mediaElement() if it existed and only using the insertionPoint if there was no mediaElement(). Now it is only calling didRemoveTrack when there is no parent, and always calling it on the insertionPoint.

Does this mean it is now ok for this to NOT call didRemoveTrack() when there is a parentNode()? I guess that's what the test below is testing.

Sorry if I'm misunderstanding.

[Dean Jackson]

Comment on attachment 176410 [details]
Patch

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

> Source/WebCore/ChangeLog:27
> +        (WebCore::HTMLTrackElement::insertedInto):
> +        (WebCore::HTMLTrackElement::removedFrom):

You might want to describe what changed here.

[Hajime Morrita]

(In reply to comment #5)
> 
> Just checking the logic here. Previously this was calling willRemoveTrack on the parent/mediaElement() if it existed and only using the insertionPoint if there was no mediaElement(). Now it is only calling didRemoveTrack when there is no parent, and always calling it on the insertionPoint.
> 
> Does this mean it is now ok for this to NOT call didRemoveTrack() when there is a parentNode()? I guess that's what the test below is testing.

Yes. 
If we call didRemoveTrack() even when the <track> has a parent, the <track> will be invalidated
when the parent <media> or its ascent is removed from the tree.
This is not desirable because we want keep <track> valid even it is not a part of the tree.
Instead of that, this patch invalidates the <track> only if it is removed from its parent <media>.

(In reply to comment #6)
> (From update of attachment 176410 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176410&action=review
> 
> > Source/WebCore/ChangeLog:27
> > +        (WebCore::HTMLTrackElement::insertedInto):
> > +        (WebCore::HTMLTrackElement::removedFrom):
> 
> You might want to describe what changed here.

Sure!

[Hajime Morrita]

Created attachment 176667 [details]
Patch

[Eric Carlson]

Comment on attachment 176667 [details]
Patch

Thank you!

[WebKit Review Bot]

Comment on attachment 176667 [details]
Patch

Clearing flags on attachment: 176667

Committed r136131: <http://trac.webkit.org/changeset/136131>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.