Bug 159087 – Binding generator should generate safe accessors for constructors used in JS builtin code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

ReadableStrem as well as RTC codes is using JS built-in.
It is often useful to have a safe access to constructors (@ReadableStream for instance).
Binding generator should be able to handle that using PrivateIdentifier/PublicIdentifier keywords.

[youenn fablet]

Created attachment 281969 [details]
Patch

[youenn fablet]

With this patch, it also becomes a bit easier to do type checks in JS-builtins like:
if(arg1 instanceof @MyClass)
   @throw...

[Alex Christensen]

Could you add a bindings test that shows the new behavior of the bindings generator?

[Adam Bergkvist]

This feature would be very useful. Thanks Youenn.

[Adam Bergkvist]

Comment on attachment 281969 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaStream.idl:32
> +    PublicIdentifier

Is it obvious that these extended attributes are referring to the constructor? Would it be more descriptive to use 'PrivateConstructorIdentifier' and 'PublicConstructorIdentifier'?

[youenn fablet]

Created attachment 282072 [details]
Patch

[youenn fablet]

(In reply to comment #3)
> Could you add a bindings test that shows the new behavior of the bindings
> generator?

I added a binding test for global object attribute.
This does not fully test these attributes on constructors though.
Maybe run-binding-tests should be beefed up and support that as well.

> > Source/WebCore/Modules/mediastream/MediaStream.idl:32
> > +    PublicIdentifier
> 
> Is it obvious that these extended attributes are referring to the
> constructor? Would it be more descriptive to use
> 'PrivateConstructorIdentifier' and 'PublicConstructorIdentifier'?

I am not opposed to that although I did not do it in the uploaded patch.
Doing so would require to convert PrivateConstructorIdentifier into PrivateIdentifier when adding attributes to the global object for each defined constructor in Source/WebCore/bindings/scripts/preprocess-idls.pl script.

[Adam Bergkvist]

(In reply to comment #7)
> > Is it obvious that these extended attributes are referring to the
> > constructor? Would it be more descriptive to use
> > 'PrivateConstructorIdentifier' and 'PublicConstructorIdentifier'?
> 
> I am not opposed to that although I did not do it in the uploaded patch.
> Doing so would require to convert PrivateConstructorIdentifier into
> PrivateIdentifier when adding attributes to the global object for each
> defined constructor in Source/WebCore/bindings/scripts/preprocess-idls.pl
> script.

I think it works as is as well.

[Alex Christensen]

Comment on attachment 282072 [details]
Patch

Sure.  We can change the names later if we're unhappy with them.

[WebKit Commit Bot]

Comment on attachment 282072 [details]
Patch

Clearing flags on attachment: 282072

Committed r202551: <http://trac.webkit.org/changeset/202551>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.