[webkit-reviews] review denied: [Bug 80696] Remove custom bindings for attribute type Array. : [Attachment 132458] patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Vineet Chaudhary (vineetc)
<rgf748 at motorola.com>'s request for review:
Bug 80696: Remove custom bindings for attribute type Array.
https://bugs.webkit.org/show_bug.cgi?id=80696

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=132458&action=review


> Source/WebCore/bindings/js/JSDOMBinding.h:286
> +	   if (vector.isEmpty())
> +	       return JSC::jsNull();

Does this mean that if vector contains no items, then null is returned instead
of []? Is it correct?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3008
> +    if ($type eq "sequence<ScriptProfile>") {
> +	   AddToImplIncludes("<runtime/JSArray.h>", $conditional);
> +	   AddToImplIncludes("JSScriptProfile.h", $conditional);
> +	   AddToImplIncludes("ScriptProfile.h", $conditional);
> +	   return "jsArray(exec, $thisValue->globalObject(), $value)";
> +    }

Shall we improve this code so that it can support a general array type? In
other words, if $type is sequence<X>, then we want to add <runtime/JSArray.h>,
JSX.h and X.h.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3776
> +    if ($type eq "sequence<ScriptProfile>") {
> +	   AddToImplIncludes("V8ScriptProfile.h");
> +	   AddToImplIncludes("ScriptProfile.h");
> +	   return "v8Array($value)";
> +    }

Ditto.

Is V8Array() already defined?

> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:46
> +#include "V8ScriptProfile.h"

Also, #include "V8sequence<ScriptProfile>.h" (appeared below) should be removed
from this file.