Bug 93097 – [JSC] Remove custom JSBindings for constructArrayBufferView()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vineet Chaudhary (vineetc)]

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

[Vineet Chaudhary (vineetc)]

Created attachment 156342 [details]
Patch

Typed Array Specification : http://www.khronos.org/registry/typedarray/specs/latest/#7

[Kentaro Hara]

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

[Vineet Chaudhary (vineetc)]

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

[Vineet Chaudhary (vineetc)]

(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 { ... }

[Kentaro Hara]

(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")?

[Vineet Chaudhary (vineetc)]

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

[Vineet Chaudhary (vineetc)]

Created attachment 156644 [details]
updatedPatch

Updated patch with support of [TypedArray]

[Kentaro Hara]

Comment on attachment 156644 [details]
updatedPatch

Looks OK. Good refactoring!

[Vineet Chaudhary (vineetc)]

Comment on attachment 156644 [details]
updatedPatch

Thanks.. For V8 Custom bindings logged new Bug93248

[Kentaro Hara]

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 ?

[WebKit Review Bot]

Comment on attachment 156644 [details]
updatedPatch

Clearing flags on attachment: 156644

Committed r124755: <http://trac.webkit.org/changeset/124755>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.