Bug 71123

Created attachment 114140 [details] Proposed patch Uploaded so the EWS bots can point out problems in ports I can't build.

<rdar://problem/10414455>

Comment on attachment 114140 [details] Proposed patch Patch doesn’t seem to apply.

Created attachment 114164 [details] Updated to apply to TOT

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

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

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

Created attachment 114196 [details] One more time.

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.

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

Created attachment 114201 [details] YAP

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

(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?

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() ?

Created attachment 114278 [details] Patch

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.

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

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.

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

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.