Bug 159087 - Binding generator should generate safe accessors for constructors used in JS builtin code
Summary: Binding generator should generate safe accessors for constructors used in JS ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-24 04:51 PDT by youenn fablet
Modified: 2016-06-28 00:05 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.07 KB, patch)
2016-06-24 05:08 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (29.59 KB, patch)
2016-06-25 06:54 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-06-24 04:51:38 PDT
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.
Comment 1 youenn fablet 2016-06-24 05:08:05 PDT
Created attachment 281969 [details]
Patch
Comment 2 youenn fablet 2016-06-24 05:10:01 PDT
With this patch, it also becomes a bit easier to do type checks in JS-builtins like:
if(arg1 instanceof @MyClass)
   @throw...
Comment 3 Alex Christensen 2016-06-24 18:10:47 PDT
Could you add a bindings test that shows the new behavior of the bindings generator?
Comment 4 Adam Bergkvist 2016-06-25 02:33:10 PDT
This feature would be very useful. Thanks Youenn.
Comment 5 Adam Bergkvist 2016-06-25 02:37:01 PDT
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'?
Comment 6 youenn fablet 2016-06-25 06:54:40 PDT
Created attachment 282072 [details]
Patch
Comment 7 youenn fablet 2016-06-25 07:00:59 PDT
(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.
Comment 8 Adam Bergkvist 2016-06-27 10:46:26 PDT
(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.
Comment 9 Alex Christensen 2016-06-27 23:33:13 PDT
Comment on attachment 282072 [details]
Patch

Sure.  We can change the names later if we're unhappy with them.
Comment 10 WebKit Commit Bot 2016-06-28 00:05:15 PDT
Comment on attachment 282072 [details]
Patch

Clearing flags on attachment: 282072

Committed r202551: <http://trac.webkit.org/changeset/202551>
Comment 11 WebKit Commit Bot 2016-06-28 00:05:22 PDT
All reviewed patches have been landed.  Closing bug.