Currently below JS*Custom.cpp files have the custom implementation of constructArrayBufferView(). We can can remove these modifying CodeGeneratorJS.pm JSInt32ArrayCustom.cpp JSUint8ClampedArrayCustom.cpp JSUint16ArrayCustom.cpp JSInt8ArrayCustom.cpp JSUint8ArrayCustom.cpp JSUint32ArrayCustom.cpp JSInt16ArrayCustom.cpp JSFloat64ArrayCustom.cpp JSFloat32ArrayCustom.cpp
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.