Bug 90338 – [V8Binding] Merging v8NumberArray() and v8NumberArrayToVector() to v8Array() and toNativeArray() respectively.

• Component: New Bugs
• Status: RESOLVED FIXED

[Vineet Chaudhary (vineetc)]

We can remove v8NumberArray() and v8NumberArrayToVector() implementaion
merging them to current v8Array() and toNativeArray() traits.

[Vineet Chaudhary (vineetc)]

Created attachment 150321 [details]
patch

[Kentaro Hara]

Comment on attachment 150321 [details]
patch

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?

[Vineet Chaudhary (vineetc)]

Comment on attachment 150321 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150321&action=review

>> 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.

Sorry I couldn't got why to slipt patch?
Both are related changes isn't it..? Or you mean JSC's changes in different patch.

>> Source/WebCore/bindings/v8/V8Binding.h:341
>> +    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.

Actually same name 'Traits' could not be used for both v8Array() and toNativeArray()
I will rename JSC's Traits to JSTraits too.

>> 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?

Ohh my apologies I missed that.

[Kentaro Hara]

Comment on attachment 150321 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150321&action=review

>>> 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.
> 
> Sorry I couldn't got why to slipt patch?
> Both are related changes isn't it..? Or you mean JSC's changes in different patch.

I've thought you can split the patch to a patch for v8NumberArray() and a patch for v8NumberArrayToVector() (I was a bit confused by V8Traits and the missing arrayNativeValue<float>().). But now it is clear to me what you are doing. Let's keep them in one patch.

>>> Source/WebCore/bindings/v8/V8Binding.h:341
>>> +    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.
> 
> Actually same name 'Traits' could not be used for both v8Array() and toNativeArray()
> I will rename JSC's Traits to JSTraits too.

Ah I got it, V8Traits for xxxV8Value() and NativeTraits for xxxNativeValue(). Then, how about V8ValueTraits and NativeValueTraits, to make it clearer?

[Vineet Chaudhary (vineetc)]

Created attachment 150367 [details]
updatedPatch

Thanks haraken for clarifying.
Updated patch with traits name V8ValueTraits and NativeValueTraits.
For JSC side changes I think we have custom call jsUnsignedLongArrayToVector() for "unsigned long",
I think we can merge this too, so I will rename JSC's traits in another patch.

[Kentaro Hara]

Comment on attachment 150367 [details]
updatedPatch

OK!

[Vineet Chaudhary (vineetc)]

Comment on attachment 150367 [details]
updatedPatch

Thank you!!

[WebKit Review Bot]

Comment on attachment 150367 [details]
updatedPatch

Clearing flags on attachment: 150367

Committed r121663: <http://trac.webkit.org/changeset/121663>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.