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
Created attachment 156940 [details] Patch
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.
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.
Created attachment 157158 [details] updatedPatch Patch for review.
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"}.
(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)?
(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...)
(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"}.
(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.
Created attachment 157197 [details] Updated Patch Updated patch using [ConstructorTemplate=TypedArray]. Also removed all [TypedArray=*] for set()s.
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.
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 :)
Comment on attachment 157230 [details] patch_for_landing OK
Comment on attachment 157230 [details] patch_for_landing Clearing flags on attachment: 157230 Committed r125042: <http://trac.webkit.org/changeset/125042>
All reviewed patches have been landed. Closing bug.