Summary: | [V8Binding] Merging v8NumberArray() and v8NumberArrayToVector() to v8Array() and toNativeArray() respectively. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vineet Chaudhary (vineetc) <code.vineet> | ||||||
Component: | New Bugs | Assignee: | 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
Vineet Chaudhary (vineetc)
2012-06-30 09:37:47 PDT
Created attachment 150321 [details]
patch
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 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 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? 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 on attachment 150367 [details]
updatedPatch
OK!
Comment on attachment 150367 [details]
updatedPatch
Thank you!!
Comment on attachment 150367 [details] updatedPatch Clearing flags on attachment: 150367 Committed r121663: <http://trac.webkit.org/changeset/121663> All reviewed patches have been landed. Closing bug. |