Bug 146593

Created attachment 256104 [details] Patch This patch creates [StaticToPrototype] as opposed to simple static functions that translate static class functions into constructor object properties. StaticToPrototype is thought to be able to create a normal JS method in the JS object prototype but that is bound to a static (class) method of the bound object.

Of course I already contemplate to update https://trac.webkit.org/wiki/WebKitIDL the moment this is landing.

What spec is this needed for?

Created attachment 256107 [details] Patch for [StaticToPrototype] Solved warning during compilation

(In reply to comment #3) > What spec is this needed for? I added the dependency with bug 146594 because I need it for that patch. I added this extended IDL attribute because of the problem I asked help for at https://lists.webkit.org/pipermail/webkit-dev/2015-July/027502.html. I was told there that the current bindings didn't support what I needed and I didn't find any better way to avoid the casting check. I assume the the casting check is needed only if you are going to use the object members but if you aren't, you can treat it as static. The new problem then was that static members become part of the constructor, not of the prototype and the spec and its tests say clearly that it has to be like that: https://streams.spec.whatwg.org/#enqueue-in-readable-stream.

Comment on attachment 256107 [details] Patch for [StaticToPrototype] View in context: https://bugs.webkit.org/attachment.cgi?id=256107&action=review > Source/WebCore/ChangeLog:10 > + method in the JS object prototype but that is bound to a static (class) method of the bound object. For the record, CountQueuingStrategy.size could also be fixed by making its constructor custom and adding a JSFunction* "size" slot to its prototype on the fly. But the IDL would not be close to what the spec describes, making the implementation harder to understand. Also the code generated by the binding generator with this patch is more readable and efficient, as illustrated by JSTestObj.cpp. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3650 > + if ($svgPropertyType and !$function->isStatic and !$function->signature->extendedAttributes->{"StaticToFunction"}) { Do we need that test? If so, should we cover it with a binding example? > Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:1412 > +glong webkit_dom_test_obj_static_to_prototype_method(WebKitDOMTestObj* self) Maybe disable "StaticToPrototype" method GObject binding? > Source/WebCore/bindings/scripts/test/ObjC/DOMTestObj.mm:1102 > +- (int)staticToPrototypeMethod Maybe disable "StaticToPrototype" method ObjC binding?

It seems unfortunate that we need custom extended attributes for a new spec. Do you know if this functionality has been brought up for inclusion in WebIDL?

Stepping back a little, another option is a 'super-custom' keyword to allow defining the C-like function directly. This may cover cases where the method works with other callee types. See https://streams.spec.whatwg.org/#rs-pipe-through Not as nice for the specific case of 'size' of course...

(In reply to comment #6) > For the record, CountQueuingStrategy.size could also be fixed by making its > constructor custom and adding a JSFunction* "size" slot to its prototype on > the fly. Its constructor is already custom and I already tried that, but it doesn't work because it only adds the method to the prototype after invoking a constructor at least once and the protype needs to be initialized before a constructor is called. Actually some tests never invoke a constructor and just check the prototype. > But the IDL would not be close to what the spec describes, making the > implementation harder to understand. I don't understand why you say that, it seems quite straightforward to me. > Also the code generated by the binding generator with this patch is more > readable and efficient, as illustrated by JSTestObj.cpp. This is true :) > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3650 > > + if ($svgPropertyType and !$function->isStatic and !$function->signature->extendedAttributes->{"StaticToFunction"}) { > > Do we need that test? If so, should we cover it with a binding example? I am not sure, I tried to be thorough by adding to the same places were you have the isStatic check as that in many ways should behave as a static. > Maybe disable "StaticToPrototype" method GObject binding? > Maybe disable "StaticToPrototype" method ObjC binding? I need to figure out how. (In reply to comment #7) > It seems unfortunate that we need custom extended attributes for a new spec. Yes, it does. But I have checked a lot of things and I couldn't find any other way to work around the castedThis check and make the idl map to static method. Custom methods don't work because they still the castedThis before mapping to object methods of the binding object and static perse doesn't work because it maps the method to the constructor instead of the prototype and going back to the first comment, I cannot initialize the prototype until the constructor is called (or at least I don't know how). > Do you know if this functionality has been brought up for inclusion in > WebIDL? Nope. Should I? Where? I would be happy to not include put this there if I knew how to map static method from the implemementation to the prototype, even by adding how many *custom* attributes and implementing whatever is needed.

(In reply to comment #8) > Stepping back a little, another option is a 'super-custom' keyword to allow > defining the C-like function directly. > This may cover cases where the method works with other callee types. See > https://streams.spec.whatwg.org/#rs-pipe-through > Not as nice for the specific case of 'size' of course... This could work too.

Comment on attachment 256107 [details] Patch for [StaticToPrototype] View in context: https://bugs.webkit.org/attachment.cgi?id=256107&action=review > Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:1418 > + WebCore::TestObj* item = WebKit::core(self); > + glong result = item->staticToPrototypeMethod(); > + return result; I'm not sure if I understood this correctly, but if staticToPrototypeMethod is supposed to be a static method, this looks wrong, as it's called as an instance method. > Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:1428 > + item->staticToPrototypeMethodWithParameters(convertedInputString); Ditto. > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:4072 > + UNUSED_PARAM(exec); This is actually used.

(In reply to comment #11) > Comment on attachment 256107 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256107&action=review > > > Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:1418 > > + WebCore::TestObj* item = WebKit::core(self); > > + glong result = item->staticToPrototypeMethod(); > > + return result; > > I'm not sure if I understood this correctly, but if staticToPrototypeMethod > is supposed to be a static method, this looks wrong, as it's called as an > instance method. Check the other static methods. Output code is the same. Are they wrong too? > > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:4072 > > + UNUSED_PARAM(exec); > > This is actually used. I would have to generate different cases for parameters or no parameters. Isn't it ok to leave it like that to avoid the warnins of the cases without parameters not to warn?

(In reply to comment #12) > (In reply to comment #11) > > Comment on attachment 256107 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=256107&action=review > > > > > Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:1418 > > > + WebCore::TestObj* item = WebKit::core(self); > > > + glong result = item->staticToPrototypeMethod(); > > > + return result; > > > > I'm not sure if I understood this correctly, but if staticToPrototypeMethod > > is supposed to be a static method, this looks wrong, as it's called as an > > instance method. > > Check the other static methods. Output code is the same. Are they wrong too? I guess you should either disable Gobj/ObjC generated code or fix the generated code. I would tend to do the first option. In any case, see CodeGenerator*.pm in WebCore/bindings/scripts folder, the SkipFunction routine for instance.

Another case where the current custom binding is falling short is for promise returning APIs. They should reject promise in lieu of raising exception, including when the callee type is not the expected one. This is now fixed for not custom promise APIs but not yet for custom ones. Having a super-custom keyword would allow fixing this easily.

Created attachment 256297 [details] Patch for [FullCustom]

(In reply to comment #15) > Created attachment 256297 [details] > Patch for [FullCustom] I pushed another version of patch for bug 146593 with [FullCustom] implementation.

It also seems that the GObject bindings are wrong for [StaticToPrototype] and they have to go and I will fix if Sam thinks [StaticToPrototype] is a better alternative to [FullCustom]. Not sure about ObjC (I'd appreciate Sam or somebody could comment about this). I'd also appreciate a proper review for any of the cases so that we can carry on with the task :)

Comment on attachment 256297 [details] Patch for [FullCustom] View in context: https://bugs.webkit.org/attachment.cgi?id=256297&action=review > Source/WebCore/ChangeLog:8 > + I added the [FullCustom] IDL extended attribute. The idea is that when using this attribute, bindings generate How about CustomAsFuntion, as opposed to Custom meaning "Custom as method"? > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:227 > + || $attribute->signature->extendedAttributes->{"FullCustom"}) { Do we have a use case for FullCustom attributes? If not, we could simplify the patch by removing support for it. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2216 > + foreach my $attribute (@nonFullCustomAttributes) { Ditto. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2772 > + foreach my $function (@nonFullCustomFunctions) { Can we rewrite this without creating the nonFullCustomFunctions array. For example by adding the "FullCustom" test within this loop? > Source/WebCore/bindings/scripts/test/ObjC/DOMTestObj.mm:945 > + I am not sure this code is doing what "FullCustom" is expected to do. I would tend to disable it until there is a use-case for it.

Created attachment 256369 [details] Patch for [StaticToPrototype] This is for [StaticToPrototype] version. Removed the generation of the GObject bindings and avoided the generation of UNUSED_PARAMS for exec in the case of methods with parameters as Carlos suggested

Created attachment 256370 [details] Patch for [StaticToPrototype] This is for [StaticToPrototype] version. Removed the generation of the GObject bindings and avoided the generation of UNUSED_PARAMS for exec in the case of methods with parameters as Carlos suggested

Created attachment 256374 [details] Patch with [CustomObjectNoBindings] This patch is for the full custom approach. I changed the attribute name into CustomObjectNoBindings

(In reply to comment #18) > > + I added the [FullCustom] IDL extended attribute. The idea is that when using this attribute, bindings generate > > How about CustomAsFuntion, as opposed to Custom meaning "Custom as method"? Yes, FullCustom is not a good attribute name, though I think CustomAsFunction could be misleading too. I tried a more concrete approach, as CustomObjectNoBindings, though I could try CustomObjectGenerateNoBindings, if it is not concrete enough. > > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:227 > > + || $attribute->signature->extendedAttributes->{"FullCustom"}) { > > Do we have a use case for FullCustom attributes? If not, we could simplify > the patch by removing support for it. No, we don't (yet), though I think we should keep the generation consistent and IMHO, not providing it would be weird. I kept it. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2772 > > + foreach my $function (@nonFullCustomFunctions) { > > Can we rewrite this without creating the nonFullCustomFunctions array. > For example by adding the "FullCustom" test within this loop? Using "break" is not an option because it fails. The other option was putting the rest of the code inside an "if" clause, but I think it would pollute the patch and would make the code a bit harder to read. Besides that, there are also some other filters like that at the code so taking into account all options, I thought this was the best given the circumstances (I would choose the "break"). > > Source/WebCore/bindings/scripts/test/ObjC/DOMTestObj.mm:945 > > + > > I am not sure this code is doing what "FullCustom" is expected to do. > I would tend to disable it until there is a use-case for it. For the case of [StaticToPrototype] and GObject, I got confirmation that it was indeed a problem to generate those bindings so I removed it completely (and fixed the generation bug). For the case of ObjC, I acknowledge that it's trivial and I can do it quickly as soon as the Apple responsibles confirm that they do have to go.

At the moment, we have two options that could land, even separately though only one would be useful for bug 146594. I would appreciate Sam or Darin (or any reviewer) to help us decide which option is preferrable, maybe to give a better opinion for the name and tell if the ObjC bindings are ok or have to go. Youenn is right that the full custom option could have wider use, but it is also true that [StaticToPrototype] generates more binding code and can be more straighforward to implement some similar things.

> > Do we have a use case for FullCustom attributes? If not, we could simplify > > the patch by removing support for it. > > No, we don't (yet), though I think we should keep the generation consistent > and IMHO, not providing it would be weird. I kept it. The binding generator is generating code for attribute getters. The binding generator is generating code for methods being called, not for method getters. The problem we have is coming from the fact that we get a method from an object, and later call the affected method. This does not seem to be an issue for not-callable attributes. > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2772 > > > + foreach my $function (@nonFullCustomFunctions) { > > > > Can we rewrite this without creating the nonFullCustomFunctions array. > > For example by adding the "FullCustom" test within this loop? > > Using "break" is not an option because it fails. The other option was > putting the rest of the code inside an "if" clause, but I think it would > pollute the patch and would make the code a bit harder to read. Besides > that, there are also some other filters like that at the code so taking into > account all options, I thought this was the best given the circumstances (I > would choose the "break"). Maybe a statement like "next if(IsCustom...);" would work?

Created attachment 257646 [details] Patch with [CustomObjectGenerateNoBindings] Changed name of the attribute to CustomObjectGenerateNoBindings.

Comment on attachment 257646 [details] Patch with [CustomObjectGenerateNoBindings] View in context: https://bugs.webkit.org/attachment.cgi?id=257646&action=review > Source/WebCore/ChangeLog:3 > + Create [StaticToPrototype] extended IDL attribute It might be good to rename the bug entry. Also, should the "r?" on the other patch be removed? > Source/WebCore/ChangeLog:8 > + I added the [CustomObjectGenerateNoBindings] IDL extended attribute. The idea is that when using this attribute, The keyword is a bit long but I do not really have any better name. or maybe just NoBindingMethod? > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:227 > + || $attribute->signature->extendedAttributes->{"CustomObjectGenerateNoBindings"}) { CustomObjectGenerateNoBindings is not needed for attributes. Can it be removed? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2216 > + } CustomObjectGenerateNoBindings is not needed for attributes. Can it be removed? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2774 > + foreach my $function (@nonCustomObjectGenerateNoBindingsFunctions) { To reduce the size of the patch, instead of building an array, can you use inside the loop something like: next if($function->signature->extendedAttributes->{"CustomObjectGenerateNoBindings"}); > Source/WebCore/bindings/scripts/test/ObjC/DOMTestObj.mm:452 > + Might want to disable binding generation for ObjC as well?

You did almost the same comments as before and I'll provide almost the same answers :) (In reply to comment #26) > It might be good to rename the bug entry. > Also, should the "r?" on the other patch be removed? I also marked the other patch as obsolete to reduce the clutter. > > Source/WebCore/ChangeLog:8 > > + I added the [CustomObjectGenerateNoBindings] IDL extended attribute. The idea is that when using this attribute, > > The keyword is a bit long but I do not really have any better name. > or maybe just NoBindingMethod? Might be. I have no preference, let's see what Geoffrey thinks. > > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:227 > > + || $attribute->signature->extendedAttributes->{"CustomObjectGenerateNoBindings"}) { > > CustomObjectGenerateNoBindings is not needed for attributes. > Can it be removed? Considering that it didn't take too much time to implement or maintain and it brings more consistency between attributes and methods, I think it is better to keep it. Of course unless Geoffrey thinks otherwise. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2774 > > + foreach my $function (@nonCustomObjectGenerateNoBindingsFunctions) { > > To reduce the size of the patch, instead of building an array, can you use > inside the loop something like: > next > if($function->signature->extendedAttributes- > >{"CustomObjectGenerateNoBindings"}); > > > Source/WebCore/bindings/scripts/test/ObjC/DOMTestObj.mm:452 > > + > > Might want to disable binding generation for ObjC as well? I disabled them for GObject because Carlos García confirmed they were wrong. I checked how the [Custom] methods are treated in ObjC and they are generated exactly the same way as [CustomObjectGenerateNoBindings]. Given my lack of knowledged of the ObjC bindings and that it is trivial to disable the generation I prefer to give Geoffrey last word.

Comment on attachment 257646 [details] Patch with [CustomObjectGenerateNoBindings] What is the difference between the [Custom] attribute and the [CustomObjectGenerateNoBindings] attribute? When should I use one or the other?

(In reply to comment #28) > Comment on attachment 257646 [details] > Patch with [CustomObjectGenerateNoBindings] > > What is the difference between the [Custom] attribute and the > [CustomObjectGenerateNoBindings] attribute? When should I use one or the > other? [Custom] is typically used when the JS to C++ parameters mapping cannot be automated. Given an IDL named Foo and its method fooMethod, [CustomObjectGenerateNoBindings] would be used if calling fooMethod on a not-JSFoo callee should not raise a TypeError exception (default behavior). When using [CustomObjectGenerateNoBindings], one needs to directly implement the jsFooPrototypeFunctionFooMethod function. When using [Custom], one needs to implement the JSFoo::fooMethod (which is called by the generated jsFooPrototypeFunctionFooMethod function). One example is https://streams.spec.whatwg.org/#rs-pipe-through where the callee can be any object having a "pipeTo" method. It also happens for bug 146594, the callee being undefined in that case.

(In reply to comment #29) > When using [CustomObjectGenerateNoBindings], one needs to directly implement > the jsFooPrototypeFunctionFooMethod function. > When using [Custom], one needs to implement the JSFoo::fooMethod (which is > called by the generated jsFooPrototypeFunctionFooMethod function). > > One example is https://streams.spec.whatwg.org/#rs-pipe-through where the > callee can be any object having a "pipeTo" method. > It also happens for bug 146594, the callee being undefined in that case. Exactly, because in the case of [Custom], jsFooPrototypeFunctionFooMethod performs some operations like a cast to this check before calling JSFoo::fooMethod, which is our main issue here. The proposed solution not only solves that issue because it allows you to implement jsFooPrototypeFunctionFooMethod direcly, but also provides you a way of adding any more or different checks if needed. We think it might be useful not only for this case, but maybe for others too.

Created attachment 257745 [details] Patch with [CustomObjectGenerateNoBindings] Simplified patch quite a lot. Removed the attributes generation, use next instead of a new array and removed skipped generation for ObjC.

(In reply to comment #31) > Simplified patch quite a lot. Removed the attributes generation, use next > instead of a new array and removed skipped generation for ObjC. Finally I honored Youenn's comment and patch is much simpler and cleaner.

Created attachment 257751 [details] Patch Mac compilation was failing because the header of the self generated file was private to the cpp instead of in the header..

Comment on attachment 257751 [details] Patch I see. I don't think "generate no bindings" is a good way to express this difference. It is strange for IDL to instruct the parser to "generate no bindings", and for the parser then to generate some bindings (see JSTestObj.h and JSTestObj.cpp) that are incomplete in a special way. Let's call this "CustomBinding". Other options that I didn't like as much: FullyCustom CustomWithoutTypeCheck

(In reply to comment #34) > Let's call this "CustomBinding". Roger!

Created attachment 257830 [details] Patch with [CustomObjectGenerateNoBindings] Changed attribute name into CustomBindings

(In reply to comment #36) > Created attachment 257830 [details] > Patch with [CustomObjectGenerateNoBindings] > > Changed attribute name into CustomBindings Why adding an "s" to CustomBinding attribute name? It seems it only applies to one method at a time.

Created attachment 257831 [details] Patch with [CustomObjectGenerateNoBindings] Corrected typo, good catch!

(In reply to comment #38) > Created attachment 257831 [details] > Patch with [CustomObjectGenerateNoBindings] > > Corrected typo, good catch! LGTM

(In reply to comment #39) > LGTM So I guess only the official r+ (or suggesting changes) is pending.

Comment on attachment 257831 [details] Patch with [CustomObjectGenerateNoBindings] r=me

Comment on attachment 257831 [details] Patch with [CustomObjectGenerateNoBindings] Clearing flags on attachment: 257831 Committed r188119: <http://trac.webkit.org/changeset/188119>

All reviewed patches have been landed. Closing bug.

(In reply to comment #43) > All reviewed patches have been landed. Closing bug. After having landed this, I updated the wiki with the documentation about the attribute https://trac.webkit.org/wiki/WebKitIDL#CustomBinding.