[webkit-reviews] review denied: [Bug 71123] Implement TextTrackList and the textTracks attribute of HTMLMediaElement : [Attachment 114355] Updated to address Sam's feedback.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 71123: Implement TextTrackList and the textTracks attribute of
HTMLMediaElement
https://bugs.webkit.org/show_bug.cgi?id=71123

Attachment 114355: Updated to address Sam's feedback.
https://bugs.webkit.org/attachment.cgi?id=114355&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.