Bug 90338

Summary: [V8Binding] Merging v8NumberArray() and v8NumberArrayToVector() to v8Array() and toNativeArray() respectively.
Product: WebKit Reporter: Vineet Chaudhary (vineetc) <code.vineet>
Component: New BugsAssignee: Vineet Chaudhary (vineetc) <code.vineet>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, haraken, japhet, jochen, scottmg, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
haraken: review-
updatedPatch none

Description Vineet Chaudhary (vineetc) 2012-06-30 09:37:47 PDT
We can remove v8NumberArray() and v8NumberArrayToVector() implementaion
merging them to current v8Array() and toNativeArray() traits.
Comment 1 Vineet Chaudhary (vineetc) 2012-06-30 09:41:27 PDT
Created attachment 150321 [details]
patch
Comment 2 Kentaro Hara 2012-06-30 09:57:03 PDT
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?
Comment 3 Vineet Chaudhary (vineetc) 2012-06-30 10:20:55 PDT
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.
Comment 4 Kentaro Hara 2012-07-01 03:29:40 PDT
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?
Comment 5 Vineet Chaudhary (vineetc) 2012-07-02 00:09:05 PDT
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.
Comment 6 Kentaro Hara 2012-07-02 00:11:28 PDT
Comment on attachment 150367 [details]
updatedPatch

OK!
Comment 7 Vineet Chaudhary (vineetc) 2012-07-02 00:16:16 PDT
Comment on attachment 150367 [details]
updatedPatch

Thank you!!
Comment 8 WebKit Review Bot 2012-07-02 01:42:46 PDT
Comment on attachment 150367 [details]
updatedPatch

Clearing flags on attachment: 150367

Committed r121663: <http://trac.webkit.org/changeset/121663>
Comment 9 WebKit Review Bot 2012-07-02 01:42:50 PDT
All reviewed patches have been landed.  Closing bug.