Bug 158777

Summary: Add bindings generator support to add a native JS function to both a 'name' and a private '@name' slot
Product: WebKit Reporter: Adam Bergkvist <adam.bergkvist>
Component: BindingsAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, eric.carlson, youennf
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 143211, 158940, 158979    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Adam Bergkvist 2016-06-15 01:43:00 PDT
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.
Comment 1 youenn fablet 2016-06-15 14:04:03 PDT
Adding such keyword should be fairly straightforward.
One can search for "Private" keyword in CodeGeneratorJS.pm to locate the lines to update.
Comment 2 youenn fablet 2016-06-16 06:46:05 PDT
Created attachment 281453 [details]
Patch
Comment 3 youenn fablet 2016-06-16 06:46:38 PDT
(In reply to comment #2)
> Created attachment 281453 [details]
> Patch

Adam, can you check whether that fits your needs?
Comment 4 Adam Bergkvist 2016-06-16 10:46:52 PDT
(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 :)
Comment 5 youenn fablet 2016-06-16 11:07:14 PDT
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.
Comment 6 Adam Bergkvist 2016-06-17 01:01:32 PDT
(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.
Comment 7 Adam Bergkvist 2016-06-20 12:05:23 PDT
Youenn, please have a look at https://bugs.webkit.org/show_bug.cgi?id=158940 for an example of code benefiting form this feature.
Comment 8 youenn fablet 2016-06-20 15:00:00 PDT
(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?
Comment 9 Eric Carlson 2016-06-21 06:21:36 PDT
(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".
Comment 10 youenn fablet 2016-06-21 06:41:18 PDT
(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.
Comment 11 Eric Carlson 2016-06-21 07:15:12 PDT
(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.
Comment 12 youenn fablet 2016-06-21 07:35:00 PDT
Created attachment 281744 [details]
Patch
Comment 13 youenn fablet 2016-06-21 08:16:39 PDT
Created attachment 281748 [details]
Patch for landing
Comment 14 youenn fablet 2016-06-21 08:18:11 PDT
Thanks for the review Eric.
I uploaded a new patch to fix the changelog.
Comment 15 WebKit Commit Bot 2016-06-21 08:45:00 PDT
Comment on attachment 281748 [details]
Patch for landing

Clearing flags on attachment: 281748

Committed r202275: <http://trac.webkit.org/changeset/202275>
Comment 16 WebKit Commit Bot 2016-06-21 08:45:05 PDT
All reviewed patches have been landed.  Closing bug.