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.
Adding such keyword should be fairly straightforward. One can search for "Private" keyword in CodeGeneratorJS.pm to locate the lines to update.
Created attachment 281453 [details] Patch
(In reply to comment #2) > Created attachment 281453 [details] > Patch Adam, can you check whether that fits your needs?
(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 :)
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.
(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.
Youenn, please have a look at https://bugs.webkit.org/show_bug.cgi?id=158940 for an example of code benefiting form this feature.
(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?
(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".
(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.
(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.
Created attachment 281744 [details] Patch
Created attachment 281748 [details] Patch for landing
Thanks for the review Eric. I uploaded a new patch to fix the changelog.
Comment on attachment 281748 [details] Patch for landing Clearing flags on attachment: 281748 Committed r202275: <http://trac.webkit.org/changeset/202275>
All reviewed patches have been landed. Closing bug.