Bug 158777 – Add bindings generator support to add a native JS function to both a 'name' and a private '@name' slot

• Component: Bindings
• Status: RESOLVED FIXED

[Adam Bergkvist]

This functionality would be useful in a case where we, for example, want to implement a function addAll() as a JS built-in based on a native function add(). This can be done today by adding a private function @privateAdd() and implement both add() and addAll() as JS built-in functions (using @privateAdd()). If we could mark a function to be added to both the 'add' and the private '@add' slots, we could just create the addAll() JS-built-in using '@add' directly, without having to touch add() further.

[youenn fablet]

Adding such keyword should be fairly straightforward.
One can search for "Private" keyword in CodeGeneratorJS.pm to locate the lines to update.

[youenn fablet]

Created attachment 281453 [details]
Patch

[youenn fablet]

(In reply to comment #2)
> Created attachment 281453 [details]
> Patch

Adam, can you check whether that fits your needs?

[Adam Bergkvist]

(In reply to comment #3)
> (In reply to comment #2)
> > Created attachment 281453 [details]
> > Patch
> 
> Adam, can you check whether that fits your needs?

It works perfectly :)

[youenn fablet]

Great!
Before going further on this one, it would be nice to see the patch that would make use of this new keyword. That would help review this one more accurately.

[Adam Bergkvist]

(In reply to comment #5)
> Great!
> Before going further on this one, it would be nice to see the patch that
> would make use of this new keyword. That would help review this one more
> accurately.

I have some code using the old approach with a private function and a JS built-in for each "public" function. I'll clean that up and attach it to this bug.

[Adam Bergkvist]

Youenn, please have a look at https://bugs.webkit.org/show_bug.cgi?id=158940 for an example of code benefiting form this feature.

[youenn fablet]

(In reply to comment #7)
> Youenn, please have a look at https://bugs.webkit.org/show_bug.cgi?id=158940
> for an example of code benefiting form this feature.

I just had a look.
I am ok going down the road of introducing a new keyword, although PrivateAlso is not the best keyword name I guess.

Also, since the private methods are called from user scripts, the type checks could be converted in assertions since the caller probably already ensured the types are ok. This is probably too early and not worth the effort right now.

cdumez, any opinion?

[Eric Carlson]

(In reply to comment #8)
> 
> I just had a look.
> I am ok going down the road of introducing a new keyword, although
> PrivateAlso is not the best keyword name I guess.
> 
I agree that "PrivateAlso" isn't a great name, I wasn't certain I knew what it meant until I read Adam's patch that uses it.

Here are some alternatives, although I am not sure any are better: "JSBuiltinVisible", "JSBuiltinAccessible", "JSBuiltinAvailable", "InternallyVisible".

[youenn fablet]

(In reply to comment #9)
> (In reply to comment #8)
> > 
> > I just had a look.
> > I am ok going down the road of introducing a new keyword, although
> > PrivateAlso is not the best keyword name I guess.
> > 
> I agree that "PrivateAlso" isn't a great name, I wasn't certain I knew what
> it meant until I read Adam's patch that uses it.
> 
> Here are some alternatives, although I am not sure any are better:
> "JSBuiltinVisible", "JSBuiltinAccessible", "JSBuiltinAvailable",
> "InternallyVisible".

We also need to find a consistent name for "Private" as well.
Private comes from the fact that the method is tied to a slot with a private identifier, vs regular methods that have a public identifier.

How about PrivateSlot or PrivateIdentifer?

To cover the case of both private and public ids, we could add Only/Both suffixes on PrivateSlot/PrivateIdentifier.

Or we could add a PublicSlot/PublicIdentifier. Adam in his patch would need to specify both keywords to expose them to JSBuiltin and user scripts.

Or we could add something like: Slot=Private, Slot=Public&Private.

[Eric Carlson]

(In reply to comment #10)
> We also need to find a consistent name for "Private" as well.
> Private comes from the fact that the method is tied to a slot with a private
> identifier, vs regular methods that have a public identifier.
> 
> How about PrivateSlot or PrivateIdentifer?
> 
> To cover the case of both private and public ids, we could add Only/Both
> suffixes on PrivateSlot/PrivateIdentifier.
> 
> Or we could add a PublicSlot/PublicIdentifier. Adam in his patch would need
> to specify both keywords to expose them to JSBuiltin and user scripts.
> 

So "[PrivateIdentifier]" means a method only available to JSBuiltins, "[PrivateIdentifier, PublicIdentifier]" means it is available to both user scripts and JSBuiltins, and "[PublicIdentifier]" is the default?

Seems logical to me.

[youenn fablet]

Created attachment 281744 [details]
Patch

[youenn fablet]

Created attachment 281748 [details]
Patch for landing

[youenn fablet]

Thanks for the review Eric.
I uploaded a new patch to fix the changelog.

[WebKit Commit Bot]

Comment on attachment 281748 [details]
Patch for landing

Clearing flags on attachment: 281748

Committed r202275: <http://trac.webkit.org/changeset/202275>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.