Summary: | [JSC] Remove custom JSBindings for constructArrayBufferView() | ||||||||
---|---|---|---|---|---|---|---|---|---|
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, ojan, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Vineet Chaudhary (vineetc)
2012-08-03 05:34:20 PDT
Created attachment 156342 [details] Patch Typed Array Specification : http://www.khronos.org/registry/typedarray/specs/latest/#7 Comment on attachment 156342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156342&action=review > Source/WebCore/ChangeLog:3 > + [JSDOMBinsding] Remove custom JSBindings for constructArrayBufferView() Typo: [JSDOMBinding] Nit: We normally use a [port name]. So [JSC] would be better. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:338 > + return "signed char" if $type eq "Int8Array"; > + return "unsigned char" if $type eq "Uint8Array" or $type eq "Uint8ClampedArray"; > + return "short" if $type eq "Int16Array"; > + return "unsigned short" if $type eq "Uint16Array"; > + return "int" if $type eq "Int32Array"; > + return "unsigned int" if $type eq "Uint32Array"; > + return "float" if $type eq "Float32Array"; > + return "double" if $type eq "Float64Array"; Instead of writing these sad lines, shall we introduce a [TypedArray] IDL attribute? By using TypedArray, we can also remove sad lines in IsTypedArrayType() and GenerateHeader(). I think that it is worth introducing a new IDL attribute. (We want to remove hard-coding from CodeGenerators.) > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3820 > + push(@$outputArray, " return JSValue::encode(JSValue());\n"); Nit: One more space before 'return'. (In reply to comment #2) > (From update of attachment 156342 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156342&action=review > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:338 > > + return "signed char" if $type eq "Int8Array"; > > Instead of writing these sad lines, shall we introduce a [TypedArray] IDL attribute? By using TypedArray, we can also remove sad lines in IsTypedArrayType() and GenerateHeader(). I think that it is worth introducing a new IDL attribute. (We want to remove hard-coding from CodeGenerators.) Ohh Oke I will check that. BTW we can remove similar toV8() calls from respective V8Bindings too. (In reply to comment #2) > Instead of writing these sad lines, shall we introduce a [TypedArray] IDL attribute? By using TypedArray, we can also remove sad lines in IsTypedArrayType() and GenerateHeader(). I think that it is worth introducing a new IDL attribute. (We want to remove hard-coding from CodeGenerators.) haraken: I tried using [TypedArray] attribute in the Float64Array.idl as below. //I guess you meant like that. interface [ ... TypedArray="double" ] Float64Array : ArrayBufferView { ... } This will solve problems from JSC bindings but as I am planning to remove toV8() call also 'TypedArray="double"' wont be helpful in that case as for v8 it uses v8::kExternalDoubleArray do you think if any common solution to this. like : interface [ ... JSTypedArray="double", V8TypedArray="kExternalDoubleArray" ] Float64Array : ArrayBufferView { ... } (In reply to comment #4) > (In reply to comment #2) > haraken: I tried using [TypedArray] attribute in the Float64Array.idl as below. //I guess you meant like that. > > interface [ > ... > TypedArray="double" > ] Float64Array : ArrayBufferView { ... } > > This will solve problems from JSC bindings but as I am planning to remove toV8() call also 'TypedArray="double"' wont be helpful in that case as for v8 it uses > v8::kExternalDoubleArray do you think if any common solution to this. > like : > > interface [ > ... > JSTypedArray="double", > V8TypedArray="kExternalDoubleArray" > ] Float64Array : ArrayBufferView { ... } Isn't it possible to modify CodeGeneratorV8.pm so that it generates the constant variable name? (i.e. "kExternal" . lcFirst($attrExt->{"TypedArray"}) . "Array")? (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #2) > > haraken: I tried using [TypedArray] attribute in the Float64Array.idl as below. //I guess you meant like that. > > > > interface [ > > ... > > TypedArray="double" > > ] Float64Array : ArrayBufferView { ... } > > > > This will solve problems from JSC bindings but as I am planning to remove toV8() call also 'TypedArray="double"' wont be helpful in that case as for v8 it uses > > v8::kExternalDoubleArray do you think if any common solution to this. > > like : > > > > interface [ > > ... > > JSTypedArray="double", > > V8TypedArray="kExternalDoubleArray" > > ] Float64Array : ArrayBufferView { ... } > > Isn't it possible to modify CodeGeneratorV8.pm so that it generates the constant variable name? (i.e. "kExternal" . lcFirst($attrExt->{"TypedArray"}) . "Array")? Thanks, Surely we can use ucfirst() again hack into for "unsigned int". Created attachment 156644 [details]
updatedPatch
Updated patch with support of [TypedArray]
Comment on attachment 156644 [details]
updatedPatch
Looks OK. Good refactoring!
Comment on attachment 156644 [details] updatedPatch Thanks.. For V8 Custom bindings logged new Bug93248 Comment on attachment 156644 [details] updatedPatch View in context: https://bugs.webkit.org/attachment.cgi?id=156644&action=review > Source/WebCore/bindings/scripts/IDLAttributes.txt:109 > +TypedArray=* Would you add an explanation to https://trac.webkit.org/wiki/WebKitIDL ? Comment on attachment 156644 [details] updatedPatch Clearing flags on attachment: 156644 Committed r124755: <http://trac.webkit.org/changeset/124755> All reviewed patches have been landed. Closing bug. |