[webkit-reviews] review denied: [Bug 80696] Remove custom bindings for attribute type Array. : [Attachment 132773] updated_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 132773: updated_patch
https://bugs.webkit.org/attachment.cgi?id=132773&action=review

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


> Source/WebCore/bindings/js/JSDOMBinding.h:295
> +    Iterable jsValueToNative(JSC::ExecState* exec, JSC::JSValue value)

This should be named toJSArray() for naming consistency.

BTW, what's the difference between toJSArray() and existing toJSSequence()?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1112
> +	   my @arrayType = split(/\W+/, $nativeType);

Please use GetArrayType()

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:969
> +    impl->setSequenceAttr(jsValueToNative(exec, value));

It seems that the argument type is inconsistent with that of V8 generated code.
What type is setSequenceAttr() expecting for the first argument?

> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:234
> +    ScriptProfile v = v8ValueToNative(value);

This is wrong. v8ValueToNative() returns Vector.

> Source/WebCore/bindings/v8/V8Binding.h:301
> +    Vector<T> v8ValueToNative(v8::Handle<v8::Value> value)

This should be named toV8Array(), for naming consistency with toInt32(),
toFloat(), toWebCoreString(), etc.