Bug 93097

Summary: [JSC] Remove custom JSBindings for constructArrayBufferView()
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, ojan, 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-08-03 05:34:20 PDT
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
Comment 1 Vineet Chaudhary (vineetc) 2012-08-03 06:01:16 PDT
Created attachment 156342 [details]
Patch

Typed Array Specification : http://www.khronos.org/registry/typedarray/specs/latest/#7
Comment 2 Kentaro Hara 2012-08-03 06:21:01 PDT
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'.
Comment 3 Vineet Chaudhary (vineetc) 2012-08-03 08:11:50 PDT
(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.
Comment 4 Vineet Chaudhary (vineetc) 2012-08-06 00:13:47 PDT
(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 { ... }
Comment 5 Kentaro Hara 2012-08-06 00:25:05 PDT
(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")?
Comment 6 Vineet Chaudhary (vineetc) 2012-08-06 00:36:15 PDT
(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".
Comment 7 Vineet Chaudhary (vineetc) 2012-08-06 03:18:55 PDT
Created attachment 156644 [details]
updatedPatch

Updated patch with support of [TypedArray]
Comment 8 Kentaro Hara 2012-08-06 03:28:32 PDT
Comment on attachment 156644 [details]
updatedPatch

Looks OK. Good refactoring!
Comment 9 Vineet Chaudhary (vineetc) 2012-08-06 04:28:32 PDT
Comment on attachment 156644 [details]
updatedPatch

Thanks.. For V8 Custom bindings logged new Bug93248
Comment 10 Kentaro Hara 2012-08-06 04:42:10 PDT
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 11 WebKit Review Bot 2012-08-06 05:02:51 PDT
Comment on attachment 156644 [details]
updatedPatch

Clearing flags on attachment: 156644

Committed r124755: <http://trac.webkit.org/changeset/124755>
Comment 12 WebKit Review Bot 2012-08-06 05:02:56 PDT
All reviewed patches have been landed.  Closing bug.