IDL functions and attributes should be JSBuiltin by default if interface is marked as JSBuiltinConstructor

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[youenn fablet]

IDL functions and attributes should be JSBuiltin by default if interface is marked as JSBuiltinConstructor.
One exception is for custom attributes/functions which should remain implemented in C++.

[youenn fablet]

Created attachment 263806 [details]
Patch

[youenn fablet]

Created attachment 263808 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 263808 [details]
Patch

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

I would also rename the custom attribute from JSBuiltinConstructor to JSBuiltinInterface since it forces not only the constructor but the whole interface to be builtin unless said otherwise.

Another thing would be ensuring that CustomConstructor also works here, maybe adding also some small test for it.

Would you be also so kind of adding Igalia to the copyright section at CodeGeneratorJS.pm since I introduced some new tag and I forgot to do it at the moment? O:-)

> Source/WebCore/ChangeLog:15
> +        No change in behavior.

What does this mean? I think there's change in behavior, isn't it? Actually a big one.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5034
> +    return 0 if $object->signature->extendedAttributes->{"Custom"};
> +    return 0 if $object->signature->extendedAttributes->{"CustomGetter"};
> +    return 0 if $object->signature->extendedAttributes->{"CustomSetter"};
> +
> +    return 1 if $object->signature->extendedAttributes->{"JSBuiltin"};
> +    return 1 if $interface->extendedAttributes->{"JSBuiltinConstructor"};

So I guess the idea is that if there is some method you want to write in C++ you set it as custom and then you implement it in C++.

I think you should add CustomBinding here too as it is even more restrictive than Custom.

> Source/WebCore/bindings/scripts/test/TestJSBuiltinConstructor.idl:38
> +    [ Custom ] void testCustomFunction();
> +    [ CustomGetter ] readonly attribute boolean testAttributeCustom;
> +    [ CustomGetter, CustomSetter ] attribute boolean testAttributeRWCustom;

If you add [ CustomBinding ] above do not forget to add it here too.

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 263808 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263808&action=review
> 
> I would also rename the custom attribute from JSBuiltinConstructor to
> JSBuiltinInterface since it forces not only the constructor but the whole
> interface to be builtin unless said otherwise.

I would not do the renaming yet since we would loose the fact that the interface has a constructor.
Following your suggestion, I think this would be good ultimately to remove JSBuiltinConstructor and have JSBuiltin at the interface level.

The current patch is a first step in that direction.
In particular, I would like ReadableStreamController and ReadableStreamReader interfaces to be JSBuiltin+CustomConstructor+NoInterfaceObject.
One or two follow-up patches should handle that.

> Would you be also so kind of adding Igalia to the copyright section at
> CodeGeneratorJS.pm since I introduced some new tag and I forgot to do it at
> the moment? O:-)

I can do it, but maybe it is better to to do that as a separate patch, mentioning the patch in question?

> > Source/WebCore/ChangeLog:15
> > +        No change in behavior.
> 
> What does this mean? I think there's change in behavior, isn't it? Actually
> a big one.

There is a change in the binding generator but no change in the currently generated code.
I can clarify this.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5034
> > +    return 0 if $object->signature->extendedAttributes->{"Custom"};
> > +    return 0 if $object->signature->extendedAttributes->{"CustomGetter"};
> > +    return 0 if $object->signature->extendedAttributes->{"CustomSetter"};
> > +
> > +    return 1 if $object->signature->extendedAttributes->{"JSBuiltin"};
> > +    return 1 if $interface->extendedAttributes->{"JSBuiltinConstructor"};
> 
> So I guess the idea is that if there is some method you want to write in C++
> you set it as custom and then you implement it in C++.

Yes.

> I think you should add CustomBinding here too as it is even more restrictive
> than Custom.

IIRC, CustomBinding is no more used since we switched to JS builtins in streams API.
Should we plan to remove it from the binding generator if we do not foresee future uses?

[Xabier Rodríguez Calvar]

(In reply to comment #4)
> I would not do the renaming yet since we would loose the fact that the
> interface has a constructor.
> Following your suggestion, I think this would be good ultimately to remove
> JSBuiltinConstructor and have JSBuiltin at the interface level.

Whichever name is fine but JSBuiltinConstructor, that's the one I think it is misleading.

> > Would you be also so kind of adding Igalia to the copyright section at
> > CodeGeneratorJS.pm since I introduced some new tag and I forgot to do it at
> > the moment? O:-)
> 
> I can do it, but maybe it is better to to do that as a separate patch,
> mentioning the patch in question?

Ok.

> > > Source/WebCore/ChangeLog:15
> > > +        No change in behavior.
> > 
> > What does this mean? I think there's change in behavior, isn't it? Actually
> > a big one.
> 
> There is a change in the binding generator but no change in the currently
> generated code.
> I can clarify this.

I think it would be useful.

> > I think you should add CustomBinding here too as it is even more restrictive
> > than Custom.
> 
> IIRC, CustomBinding is no more used since we switched to JS builtins in
> streams API.
> Should we plan to remove it from the binding generator if we do not foresee
> future uses?

As long as it isn't removed yet, I think it would be interesting to have it here for coherence unless it creates issues.

If it doesn't create any issue, I'd keep it as I think it might be useful for anybody in the future. It is not like dead code that gets compiled, if you don't use it nothing gets generated with it.

[Csaba Osztrogonác]

Comment on attachment 263808 [details]
Patch

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

> Source/WebCore/bindings/scripts/test/JS/JSTestJSBuiltinConstructor.cpp:158
> +EncodedJSValue jsTestJSBuiltinConstructorTestAttributeCustom(ExecState* state, JSObject* slotBase, EncodedJSValue thisValue, PropertyName)
> +{
> +    UNUSED_PARAM(state);
> +    UNUSED_PARAM(slotBase);
> +    UNUSED_PARAM(thisValue);
> +    auto* castedThis = jsCast<JSTestJSBuiltinConstructor*>(slotBase);
> +    return JSValue::encode(castedThis->testAttributeCustom(*state));
> +}

state and slotBase parameters are used. thisValue is really unused, 
but in this case omitting the parameter name would be better.

> Source/WebCore/bindings/scripts/test/JS/JSTestJSBuiltinConstructor.cpp:168
> +EncodedJSValue jsTestJSBuiltinConstructorTestAttributeRWCustom(ExecState* state, JSObject* slotBase, EncodedJSValue thisValue, PropertyName)
> +{
> +    UNUSED_PARAM(state);
> +    UNUSED_PARAM(slotBase);
> +    UNUSED_PARAM(thisValue);
> +    auto* castedThis = jsCast<JSTestJSBuiltinConstructor*>(slotBase);
> +    return JSValue::encode(castedThis->testAttributeRWCustom(*state));
> +}

ditto

> Source/WebCore/bindings/scripts/test/JS/JSTestJSBuiltinConstructor.cpp:188
> +void setJSTestJSBuiltinConstructorTestAttributeRWCustom(ExecState* state, JSObject* baseObject, EncodedJSValue thisValue, EncodedJSValue encodedValue)
> +{
> +    JSValue value = JSValue::decode(encodedValue);
> +    UNUSED_PARAM(baseObject);
> +    UNUSED_PARAM(thisValue);
> +    auto* castedThis = jsCast<JSTestJSBuiltinConstructor*>(baseObject);
> +    UNUSED_PARAM(thisValue);
> +    UNUSED_PARAM(state);
> +    castedThis->setTestAttributeRWCustom(*state, value);
> +}

ditto

[youenn fablet]

Good catch!
I filed bug 150446 to keep track of it.

[youenn fablet]

Created attachment 264010 [details]
Making all changes at once

[youenn fablet]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 263808 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=263808&action=review
> > 
> > I would also rename the custom attribute from JSBuiltinConstructor to
> > JSBuiltinInterface since it forces not only the constructor but the whole
> > interface to be builtin unless said otherwise.
> 
> I would not do the renaming yet since we would loose the fact that the
> interface has a constructor.
> Following your suggestion, I think this would be good ultimately to remove
> JSBuiltinConstructor and have JSBuiltin at the interface level.
> 
> The current patch is a first step in that direction.
> In particular, I would like ReadableStreamController and
> ReadableStreamReader interfaces to be
> JSBuiltin+CustomConstructor+NoInterfaceObject.
> One or two follow-up patches should handle that.

I did tried to do everything as one patch and this might be actually be ok, as uploaded.
Only JSBuiltIn keyword is surviving in this patch.
At the interface level, JSBuiltIn means that DOM class is not needed and if there is a constructor (not custom) it should call a JS built-in init function.

If it is better to do that as two patches, previous patch remains valid.

I did not added CustomBinding handling, since I am not sure we should continue supporting it.

[Darin Adler]

Comment on attachment 264010 [details]
Making all changes at once

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

> Source/WebCore/Modules/streams/ReadableStreamController.idl:37
>      [JSBuiltin] void enqueue([Default=Undefined] optional any chunk);

Patch doesn’t remove [JSBuiltin] from the methods and attributes in this interface. Why?

> Source/WebCore/Modules/streams/ReadableStreamReader.idl:37
>      [JSBuiltin] Promise read();

Patch doesn’t remove [JSBuiltin] from the methods and attributes in this interface. Why?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5031
> +    return 0 if $object->signature->extendedAttributes->{"CustomGetter"};
> +    return 0 if $object->signature->extendedAttributes->{"CustomSetter"};

I don’t think this is right. It will make both the getter and setter be non-JS-built-in even if only one or the other is custom. We should consider a test case where the getter is JavaScript but the setter is C++ code and vice versa. Or we can just leave this broken until we need it, since it’s only used within the WebKit project.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5034
> +    return 1 if $object->signature->extendedAttributes->{"JSBuiltin"};
> +    return 1 if $interface->extendedAttributes->{"JSBuiltin"};

What if the class is JSBuiltin, but we don’t want this attribute or method to be? No keyword to mark a function as an exception? Or maybe that is exactly what Custom means in those cases?

[youenn fablet]

Thanks for the review.

> > Source/WebCore/Modules/streams/ReadableStreamReader.idl:37
> >      [JSBuiltin] Promise read();
> 
> Patch doesn’t remove [JSBuiltin] from the methods and attributes in this
> interface. Why?

Will fix that in landing patch.

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5031
> > +    return 0 if $object->signature->extendedAttributes->{"CustomGetter"};
> > +    return 0 if $object->signature->extendedAttributes->{"CustomSetter"};
> 
> I don’t think this is right. It will make both the getter and setter be
> non-JS-built-in even if only one or the other is custom. We should consider
> a test case where the getter is JavaScript but the setter is C++ code and
> vice versa. Or we can just leave this broken until we need it, since it’s
> only used within the WebKit project.

In the binding generator, some parts are already ready for setters.
AFAICT, JSBuiltin attribute setters are not supported yet within JSC (see reifyStaticAccessor in Lookup.cpp).
I would prefer to not tackle it until we actually have a usecase for it.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5034
> > +    return 1 if $object->signature->extendedAttributes->{"JSBuiltin"};
> > +    return 1 if $interface->extendedAttributes->{"JSBuiltin"};
> 
> What if the class is JSBuiltin, but we don’t want this attribute or method
> to be? No keyword to mark a function as an exception? Or maybe that is
> exactly what Custom means in those cases?

Yes, Custom should be used.
This is matching pretty well since JSBuiltin classes have no DOM class and Custom leads to using JSXX methods.
I will add documentation in https://trac.webkit.org/wiki/WebKitIDL about this and attribute setters missing support.

[youenn fablet]

Created attachment 264570 [details]
Cleaning IDLs

[WebKit Commit Bot]

Comment on attachment 264570 [details]
Cleaning IDLs

Clearing flags on attachment: 264570

Committed r191885: <http://trac.webkit.org/changeset/191885>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.