[webkit-reviews] review denied: [Bug 62887] JavaScript access to text tracks from HTMLMediaElement : [Attachment 106812] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Anna Cavender <annacc at chromium.org>'s
request for review:
Bug 62887: JavaScript access to text tracks from HTMLMediaElement
https://bugs.webkit.org/show_bug.cgi?id=62887

Attachment 106812: Patch
https://bugs.webkit.org/attachment.cgi?id=106812&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=106812&action=review


As this is adding bindings code which is easily testable, please include the
tests which test the things you are adding in this patch.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:634
> +#if ENABLE(VIDEO_TRACK)
> +JSValue JSDOMWindow::textTrackCue(ExecState* exec) const
> +{
> +    return getDOMConstructor<JSTextTrackCueConstructor>(exec, this);
> +}
> +#endif

This does not need to be custom.

> Source/WebCore/bindings/js/JSHTMLMediaElementCustom.cpp:52
> +    MarkedArgumentBuffer list;
> +    for (size_t i = 0; i < textTracks->size(); i++)
> +	   list.append(toJS(exec, globalObject(), (*textTracks)[i].get()));
> +    return constructArray(exec, list);

This will return a normal array, which is mutable.  The spec says this should
be "a platform array object for objects of type TextTrack that is fixed length
and read only".  It also says "The same object must be returned each time the
attribute is accessed", which this does not.

> Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp:50
> +    RefPtr<TextTrackCue> cue;

This variable is not used until much farther down and is not used in the outer
scope.

> Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp:52
> +    if (exec->argumentCount()) {

This would probably be cleaner as an early return.

> Source/WebCore/bindings/js/JSWorkerContextCustom.cpp:115
> +JSValue JSWorkerContext::textTrackCue(ExecState* exec) const
> +{
> +    return getDOMConstructor<JSTextTrackCueConstructor>(exec, this);
> +}
> +#endif

Is this code meant to work with Workers?

> Source/WebCore/page/DOMWindow.idl:606
> +	   attribute [JSCCustomGetter] TextTrackCueConstructor TextTrackCue; //
Usable with the new operator

There is no reason to mark this Custom.  The Constructor suffix should be
enough.