Bug 71123 – Implement TextTrackList and the textTracks attribute of HTMLMediaElement

• Component: Media
• Status: RESOLVED FIXED

[Anna Cavender]

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-media-texttracks

[Eric Carlson]

Created attachment 114140 [details]
Proposed patch

Uploaded so the EWS bots can point out problems in ports I can't build.

[Radar WebKit Bug Importer]

<rdar://problem/10414455>

[Darin Adler]

Comment on attachment 114140 [details]
Proposed patch

Patch doesn’t seem to apply.

[Eric Carlson]

Created attachment 114164 [details]
Updated to apply to TOT

[Early Warning System Bot]

Comment on attachment 114164 [details]
Updated to apply to TOT

Attachment 114164 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10374217

[Eric Carlson]

Created attachment 114175 [details]
Updated to compile when ENABLE_VIDEO_TRACK is not defined

[WebKit Review Bot]

Comment on attachment 114175 [details]
Updated to compile when ENABLE_VIDEO_TRACK is not defined

Attachment 114175 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10376246

[Eric Carlson]

Created attachment 114196 [details]
One more time.

[WebKit Review Bot]

Attachment 114196 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1

Source/WebCore/bindings/js/JSTextTrackListCustom.cpp:47:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 114196 [details]
One more time.

Attachment 114196 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10375317

[Eric Carlson]

Created attachment 114201 [details]
YAP

[WebKit Review Bot]

Comment on attachment 114201 [details]
YAP

Attachment 114201 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10382104

[Eric Carlson]

(In reply to comment #12)
> (From update of attachment 114201 [details])
> Attachment 114201 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10382104

out/Release/obj/gen/webcore/bindings/V8HTMLTrackElement.cpp: In function 'v8::Handle<v8::Value> WebCore::HTMLTrackElementInternal::trackAttrGetter(v8::Local<v8::String>, const v8::AccessorInfo&)':
out/Release/obj/gen/webcore/bindings/V8HTMLTrackElement.cpp:130: error: call of overloaded 'toV8(WTF::PassRefPtr<WebCore::LoadableTextTrack>)' is ambiguous
out/Release/obj/gen/webkit/bindings/V8Event.h:72: note: candidates are: ...

HTMLTrackElement.idl didn't change, why does V8HTMLTrackElement.cpp no longer compile?

[Adam Barth]

Comment on attachment 114201 [details]
YAP

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

> Source/WebCore/html/HTMLTrackElement.cpp:154
> +    return m_track;

Do you mean return m_track.release() ?

[Eric Carlson]

Created attachment 114278 [details]
Patch

[Sam Weinig]

Comment on attachment 114278 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:2046
> +PassRefPtr<TextTrackList> HTMLMediaElement::textTracks() const 

This probably wants to be a raw pointer return type.

> Source/WebCore/html/HTMLMediaElement.h:249
> +    enum InvalidUrlAction { DoNothing, Complain };

Url should be spelled URL.

> Source/WebCore/html/HTMLTrackElement.cpp:157
> +PassRefPtr<TextTrack> HTMLTrackElement::track()

This should probably return a raw pointer.

> Source/WebCore/html/HTMLTrackElement.cpp:160
> +    RefPtr<TextTrack> track = loadableTrack();
> +    return track.release();

This can just be return loadableTrack();

> Source/WebCore/html/HTMLTrackElement.h:77
> +    // TextTrackLoadingClient

I think this is just a TextTrackClient now.

> Source/WebCore/html/HTMLTrackElement.h:88
> +    RefPtr<ScriptExecutionContext> m_context;

I am not sure you need to store this.  The Document associate with the element should be the ScriptExecutionContext to use.

> Source/WebCore/html/LoadableTextTrack.h:47
> +    virtual void loadingCompleted(LoadableTextTrack*, bool /* loadingFailed */) { }

Maybe didCompleteLoad()?

> Source/WebCore/html/LoadableTextTrack.h:75
> +    RefPtr<ScriptExecutionContext> m_context;

You should probably be able to get to this through the m_trackElement.

> Source/WebCore/html/track/TextTrackList.h:31
> +#include "ActiveDOMObject.h"

I don't think you use this anymore.

[Eric Carlson]

Created attachment 114355 [details]
Updated to address Sam's feedback.

[Geoffrey Garen]

Comment on attachment 114355 [details]
Updated to address Sam's feedback.

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

r- because the JS marking stuff is not quite right. I'm sorry this API turned out to be so complicated. Sam and I are working on making it better.

> Source/WebCore/bindings/js/JSTextTrackListCustom.cpp:45
> +    if (textTrackList->hasEventListeners())
> +        return true;
> +    if (jsTextTrackList->hasCustomProperties())
> +        return true;

This is a memory leak. 

It is valid to say "if (!x) return false" here to optimize away a wrapper. But saying "if (x) return true" will cause a wrapper with custom properties or event listeners to be immortal.

> Source/WebCore/bindings/js/JSTextTrackListCustom.cpp:47
> +    return visitor.containsOpaqueRoot(root(textTrackList->owner()));

You also need a custom mark function that calls visitor.addOpaqueRoot(root(...)), so that, if this list is marked, it keeps the rest of the DOM alive, since the list has accessors to the rest of the DOM.

[Eric Carlson]

Created attachment 114565 [details]
Addressing Geoff's comments.

[Sam Weinig]

Comment on attachment 114565 [details]
Addressing Geoff's comments.

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

> Source/WebCore/html/HTMLTrackElement.cpp:46
> +    , m_track(0)

You don't have to initialize this since it is in a RefPtr.

[Eric Carlson]

http://trac.webkit.org/changeset/99984