Bug 149518 – [Streams API] Add support for private WebCore JS builtins functions

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

[youenn fablet]

Private JS builtin functions are needed for ReadableStream JS builtin implementation.

[youenn fablet]

Created attachment 261864 [details]
Patch

[WebKit Commit Bot]

Attachment 261864 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMWindowBase.cpp:91:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 261864 [details]
Patch

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

> Source/JavaScriptCore/builtins/BuiltinNames.h:115
> +    m_privateToPublicMap.add(privateName.impl(), &publicName);

Might be good to add an ASSERT above to ensure name collision does not happen.

[Xabier Rodríguez Calvar]

(In reply to comment #3)
> Might be good to add an ASSERT above to ensure name collision does not
> happen.

Yep, definitely needed. Actually I remember trying to figure out a crash because of a collision so it is very needed.

The rest of the shallow review looks good. Only one comment you knew I was going to make, that is why don't you separate into the generation and the actual tee skeleton.

[youenn fablet]

(In reply to comment #4)
> (In reply to comment #3)
> > Might be good to add an ASSERT above to ensure name collision does not
> > happen.
> 
> Yep, definitely needed. Actually I remember trying to figure out a crash
> because of a collision so it is very needed.
> 
> The rest of the shallow review looks good. Only one comment you knew I was
> going to make, that is why don't you separate into the generation and the
> actual tee skeleton.

Thanks.
As of tee skeleton, you are right, I could have split it.
It feels a bit safer to validate everything works fine on all bots at the time the binding generator is updated.

[youenn fablet]

Ping review, this is the last patch messing with JSC before being able to do pure JS Builtins DOM classes.

[Darin Adler]

Comment on attachment 261864 [details]
Patch

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

> Source/JavaScriptCore/runtime/CommonIdentifiers.cpp:86
> +}
> +
>  

Extra blank line here.

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:362
> +        JS_EXPORT_PRIVATE void appendExternalName(const Identifier& publicName, const Identifier& privateName);

This API is a bit dangerous and difficult to use correctly. These maps hold pointers to Identifier objects. That means the Identifier itself has to survive, not just the reference counted object the Identifier holds. That might not be clear to the caller. Probably even needs a comment.

> Source/WebCore/bindings/js/JSDOMWindowBase.h:88
> +#if ENABLE(STREAMS_API)
> +        ReadableStreamInternalsBuiltinFunctions m_readableStreamFunctions;
> +#endif

Does this really need to be done like this? It’s not good that we’re going to need to add one of these every time we add another class that uses built-ins. Can we minimize the number of separate places that we have to list ReadableStream explicitly? Instead we could simply have something that is for all built-in functions from all enabled WebCore features and have the #if be done as part of the table generation code instead of at each call site.

I count 10 different places that each had to list this explicitly. That’s not good for the future.

If JSDOMWindowBase and WebCoreJSClientData are going to need to include all of them, then what’s the value of generating each class’s list of names in a separate file?

> Source/WebCore/generate-js-builtins:140
> +class ${Filename}BuiltinFunctions
> +{

We’d normally put the brace on the line before.

[youenn fablet]

Thanks for the review!

(In reply to comment #7)
> Comment on attachment 261864 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261864&action=review
> 
> > Source/JavaScriptCore/runtime/CommonIdentifiers.cpp:86
> > +}
> > +
> >  
> 
> Extra blank line here.

OK

> 
> > Source/JavaScriptCore/runtime/CommonIdentifiers.h:362
> > +        JS_EXPORT_PRIVATE void appendExternalName(const Identifier& publicName, const Identifier& privateName);
> 
> This API is a bit dangerous and difficult to use correctly. These maps hold
> pointers to Identifier objects. That means the Identifier itself has to
> survive, not just the reference counted object the Identifier holds. That
> might not be clear to the caller. Probably even needs a comment.

Agreed.
I will add a comment.

> > Source/WebCore/bindings/js/JSDOMWindowBase.h:88
> > +#if ENABLE(STREAMS_API)
> > +        ReadableStreamInternalsBuiltinFunctions m_readableStreamFunctions;
> > +#endif
> 
> Does this really need to be done like this? It’s not good that we’re going
> to need to add one of these every time we add another class that uses
> built-ins. Can we minimize the number of separate places that we have to
> list ReadableStream explicitly? Instead we could simply have something that
> is for all built-in functions from all enabled WebCore features and have the
> #if be done as part of the table generation code instead of at each call
> site.
> I count 10 different places that each had to list this explicitly. That’s
> not good for the future.
> 
> If JSDOMWindowBase and WebCoreJSClientData are going to need to include all
> of them, then what’s the value of generating each class’s list of names in a
> separate file?

There are two parameters that need to be handled:
- whether a given js file should be #if ENABLE(...)
- whether a given js file contains functions mapped to WebIDL (owned by JSXXPrototype) or internal functions (owned by the global object).

The first one might be handled by the js generator by including this information as a comment in the js file.
For the second, the same could be done. Or this could be done as part of the build system. Or it could be done by hand since this might happen less often.
Let's do that after ReadableStream is shipped as a refactoring step.

> 
> > Source/WebCore/generate-js-builtins:140
> > +class ${Filename}BuiltinFunctions
> > +{
> 
> We’d normally put the brace on the line before.

OK

[youenn fablet]

Created attachment 262239 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 262239 [details]
Patch for landing

Rejecting attachment 262239 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 262239, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ore/generate-js-builtins
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/streams/reference-implementation/readable-stream-expected.txt
patching file LayoutTests/streams/reference-implementation/readable-stream-tee-expected.txt
patching file LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/231568

[youenn fablet]

Created attachment 262240 [details]
Rebasing for landing

[WebKit Commit Bot]

Attachment 262240 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMWindowBase.cpp:93:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 262246 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 262246 [details]
Patch

Clearing flags on attachment: 262246

Committed r190401: <http://trac.webkit.org/changeset/190401>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

> I count 10 different places that each had to list this explicitly. That’s
> not good for the future.
> 
> If JSDOMWindowBase and WebCoreJSClientData are going to need to include all
> of them, then what’s the value of generating each class’s list of names in a
> separate file?

Filed bug 149751 to keep track of it.