Bug 93371 – Remove All Custom binding code for TypedArray.

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

[Vineet Chaudhary (vineetc)]

Currently below listed files have Custom bindings, and can be removed completely with help of [TypedArray] attribute.

V8/JSInt32ArrayCustom.cpp
V8/JSUint8ClampedArrayCustom.cpp
V8/JSUint16ArrayCustom.cpp
V8/JSInt8ArrayCustom.cpp 
V8/JSUint8ArrayCustom.cpp
V8/JSUint32ArrayCustom.cpp
V8/JSInt16ArrayCustom.cpp 
V8/JSFloat64ArrayCustom.cpp
V8/JSFloat32ArrayCustom.cpp

[Vineet Chaudhary (vineetc)]

Created attachment 156940 [details]
Patch

[Vineet Chaudhary (vineetc)]

I have attached rough patch to get initial comments.

There can be multiple approach for removing custom setCallback() from V8 binding and set() call from JSC binding
1. That current patch does ie. calling setWebGLArrayHelper() directly.
2  We can move implementation of set() to JSArrayBufferViewHelper.h and setCallback() to V8ArrayBufferViewCustom.h

Though second solution looks good but I am not sure of its implication.

[Vineet Chaudhary (vineetc)]

Comment on attachment 156940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156940&action=review

> Source/WebCore/ChangeLog:13
> +        * bindings/js/JSFloat32ArrayCustom.cpp: Removed custom bindings.

We shall be able to delete these files.

> Source/WebCore/ChangeLog:28
> +        * bindings/scripts/test/CPP/WebDOMFloat64Array.cpp:

I have yet to work for other bindings too.

> Source/WebCore/ChangeLog:50
> +        * bindings/v8/custom/V8Float32ArrayCustom.cpp: Removed custom bindings.

We shall be able to delete these files.

[Vineet Chaudhary (vineetc)]

Created attachment 157158 [details]
updatedPatch

Patch for review.

[Kentaro Hara]

Comment on attachment 157158 [details]
updatedPatch

Marking r-, but the patch looks great.

- Remove [CustomConstructor] from all the IDLs, because they are no longer "custom". Instead, let's introduce [ConstructorTemplate=TypedArray]. Look at the implementation of [ConstructorTemplate=Event].

- Remove [TypedArray=...] from all set()s. Given that you are already specifying [TypedArray=...] on the interface, CodeGenerators can use it. Specifically, CodeGenerators can use $dataNode->extendedAttributes->{"TypedArray"} instead of $function->signature->extendedAttributes->{"TypedArray"}.

[Vineet Chaudhary (vineetc)]

(In reply to comment #5)
> (From update of attachment 157158 [details])
> Marking r-, but the patch looks great.
> 
> - Remove [CustomConstructor] from all the IDLs, because they are no longer "custom". Instead, let's introduce [ConstructorTemplate=TypedArray]. Look at the implementation of [ConstructorTemplate=Event].
> 
> - Remove [TypedArray=...] from all set()s. Given that you are already specifying [TypedArray=...] on the interface, CodeGenerators can use it. Specifically, CodeGenerators can use $dataNode->extendedAttributes->{"TypedArray"} instead of $function->signature->extendedAttributes->{"TypedArray"}.

Thanks for review.

Shall we remove [CustomIndexedSetter] too as it is also not custom anymore, any concerns/thoughts? Shall we do it in separate patch(before/after this patch)?

[Kentaro Hara]

(In reply to comment #6)
> Shall we remove [CustomIndexedSetter] too as it is also not custom anymore, any concerns/thoughts? Shall we do it in separate patch(before/after this patch)?

Let's do it in a separate patch. (Actually CodeGenerators around indexed properties and named properties are full of bugs, escpecially in CodeGeneratorV8.pm...)

[Vineet Chaudhary (vineetc)]

(In reply to comment #5)
> (From update of attachment 157158 [details])
> Marking r-, but the patch looks great.
> 
> - Remove [CustomConstructor] from all the IDLs, because they are no longer "custom". Instead, let's introduce [ConstructorTemplate=TypedArray]. Look at the implementation of [ConstructorTemplate=Event].

Actually removing [CustomConstructor] would be tricky eg. constructJSMutationObserver() has little different implementation we have to make common.
Can we land this patch first with below modification("Remove [TypedArray=...] from all set()s") then I will clean up code.

> - Remove [TypedArray=...] from all set()s. Given that you are already specifying [TypedArray=...] on the interface, CodeGenerators can use it. Specifically, CodeGenerators can use $dataNode->extendedAttributes->{"TypedArray"} instead of $function->signature->extendedAttributes->{"TypedArray"}.

[Kentaro Hara]

(In reply to comment #8)
> > - Remove [CustomConstructor] from all the IDLs, because they are no longer "custom". Instead, let's introduce [ConstructorTemplate=TypedArray]. Look at the implementation of [ConstructorTemplate=Event].
> 
> Actually removing [CustomConstructor] would be tricky eg. constructJSMutationObserver() has little different implementation we have to make common.
> Can we land this patch first with below modification("Remove [TypedArray=...] from all set()s") then I will clean up code.

Sorry, I meant, you can remove [CustomConstructor] from all the WebGL related IDLs you are changing in this patch.

[Vineet Chaudhary (vineetc)]

Created attachment 157197 [details]
Updated Patch

Updated patch using [ConstructorTemplate=TypedArray].
Also removed all [TypedArray=*] for set()s.

[Kentaro Hara]

Comment on attachment 157197 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157197&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2218
> -                    push(@implContent, "    $implType* impl = static_cast<$implType*>(castedThis->impl());\n");
> +                    if (!($function->signature->name eq "set" and $dataNode->extendedAttributes->{"TypedArray"})) {
> +                        push(@implContent, "    $implType* impl = static_cast<$implType*>(castedThis->impl());\n");
> +                    }

Just like CodeGeneratorV8.pm, can you generate set() method here and early return? Specifically, like this:

  if ($function->signature->name eq "set" and $dataNode->extendedAttributes->{"TypedArray"}) {
    push(@implContent, "...");
    return;
  }

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2253
> -                        GenerateImplementationFunctionCall($function, $functionString, "    ", $svgPropertyType, $implClassName);
> +                        if ($function->signature->name eq "set" and $dataNode->extendedAttributes->{"TypedArray"}) {
> +                            my $viewType = $dataNode->extendedAttributes->{"TypedArray"};
> +                            push(@implContent, "    return JSValue::encode(setWebGLArrayHelper<$implType, $viewType>(exec, castedThis->impl()));\n");
> +                        } else {
> +                            GenerateImplementationFunctionCall($function, $functionString, "    ", $svgPropertyType, $implClassName);
> +                        }

With the early return, this change is not needed.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3750
> +            if ($dataNode->extendedAttributes->{"CustomIndexedSetter"}) {

As you mentioned, let's remove it in a follow-up patch.

> Source/WebCore/bindings/scripts/IDLAttributes.txt:33
> -ConstructorTemplate=Event
> +ConstructorTemplate=*

ConstructorTemplate=Event|TypedArray

Please add the explanation to the WebKitIDL wiki after landing the patch.

[Vineet Chaudhary (vineetc)]

Created attachment 157230 [details]
patch_for_landing

(In reply to comment #11)
> (From update of attachment 157197 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157197&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2218
> > -                    push(@implContent, "    $implType* impl = static_cast<$implType*>(castedThis->impl());\n");
> > +                    if (!($function->signature->name eq "set" and $dataNode->extendedAttributes->{"TypedArray"})) {
> > +                        push(@implContent, "    $implType* impl = static_cast<$implType*>(castedThis->impl());\n");
> > +                    }
> 
> Just like CodeGeneratorV8.pm, can you generate set() method here and early return? Specifically, like this:

Done! 
 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3750
> > +            if ($dataNode->extendedAttributes->{"CustomIndexedSetter"}) {
> 
> As you mentioned, let's remove it in a follow-up patch.
>
Yes sure!
 
> > Source/WebCore/bindings/scripts/IDLAttributes.txt:33
> > -ConstructorTemplate=Event
> > +ConstructorTemplate=*
> 
> ConstructorTemplate=Event|TypedArray

Thanks. Done.

> Please add the explanation to the WebKitIDL wiki after landing the patch.

Yes sure. Thanks :)

[Kentaro Hara]

Comment on attachment 157230 [details]
patch_for_landing

OK

[WebKit Review Bot]

Comment on attachment 157230 [details]
patch_for_landing

Clearing flags on attachment: 157230

Committed r125042: <http://trac.webkit.org/changeset/125042>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.