[webkit-reviews] review denied: [Bug 90338] [V8Binding] Merging v8NumberArray() and v8NumberArrayToVector() to v8Array() and toNativeArray() respectively. : [Attachment 150321] 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 90338: [V8Binding] Merging v8NumberArray() and v8NumberArrayToVector() to
v8Array() and toNativeArray() respectively.
https://bugs.webkit.org/show_bug.cgi?id=90338

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

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


Looks like a good refactoring.

> Source/WebCore/ChangeLog:3
> +	   [V8Binding] Merging v8NumberArray()/v8NumberArrayToVector() to
v8Array()/toNativeArray() respectively.

Shall we split this into two patches? This patch is doing two orthogonal
non-trivial changes.

> Source/WebCore/bindings/v8/V8Binding.h:341
> -    struct Traits {
> +    struct V8Traits {

I am neutral to the rename (Is there any issue with the name 'Traits'?). At
least, if you want to rename it, you might also want to rename JSC's Traits to
JSTraits, and rename V8's NativeTraits to V8NativeTraits.

> Source/WebCore/bindings/v8/V8Binding.h:416
> +	       result.append(TraitsType::arrayNativeValue(array, i));

Where is arrayNativeValue() for float and double (which will return
array->Get(v8Integer(i))->NumberValue()) defined?