Bug 147556 – [Streams API] Implement ReadableStream pipeThrough

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

[youenn fablet]

We should implement https://streams.spec.whatwg.org/#rbs-pipe-through, in particular the ability for this function to be called on any object having a pipeTo method.

[youenn fablet]

Created attachment 258051 [details]
Patch using CustomBinding

[youenn fablet]

This patch illustrates the use of CustomBinding in the context of ReadableStream.pipeThrough.

Some additional error tests might be good to add (exception raiser getters for pipeTo, readablestream and writablestream).

[youenn fablet]

Created attachment 258487 [details]
Adding some more tests

[youenn fablet]

Created attachment 260866 [details]
JS builtin within WebCore

[youenn fablet]

Comment on attachment 260866 [details]
JS builtin within WebCore

Putting the patch as r?
The overall direction is:
- Update JSC builtins infrastructure to make it applicable to WebCore
- Update the IDL generator to automate the binding between IDL methods and WebCore JS builtins
Future patches would add support for IDL builtin constructor/attributes and private functions/names.
Any early feedback on the approach well appreciated.

[Xabier Rodríguez Calvar]

It looks very good! I would split it into two patches, one for the generation of the IDL attribute and another for the piping stuff.

[youenn fablet]

(In reply to comment #6)
> It looks very good! I would split it into two patches, one for the
> generation of the IDL attribute and another for the piping stuff.

Thanks.
It could be split but it would be a bit more work to ensure that the first patch (support for JS builtins within WebCore) is tested.

I think there is some more work to iron this patch: generated code could be tightened a bit with some const, macro handling/definition could be refactored a bit better as well.

Before doing that, I would prefer to validate the approach, namely:
- Generating one cpp/h file per JS builtin WebCore file
- Generating helper class to take the role of JSC::BuiltinExecutables/BuiltinNames
- Integration of that helper class in WebCore (currently hooked in WebCoreJSClientData)
- Integration of IDL method as JS builtin using JSBuiltin keyword (JS function is directly owned by the JSXXPrototype class and not by JSGlobalObject).

The approach would then for each class XX that wants to use builtins to have a XX.idl file and a XX.js file that would implement the JSBuiltin IDL methods.
 
Private JS builtin functions (which might be hooked through JSDOMWindowBase) would be located in separate files as their symbols would be exported to the JSC VM, which is not needed for JS builtins implementing the IDL methods.

[youenn fablet]

Created attachment 261314 [details]
Some clean up

[Darin Adler]

Comment on attachment 261314 [details]
Some clean up

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

I’m not sure we’ve properly thought through what to do for bindings like Objective-C. I would be tempted to say that by default a JSBuiltin should be entirely left out of the other bindings. Or are we going to teach the other bindings how to call through to JavaScript? ReadableStream is in a strange state if we try to make an Objective-C binding for it.

We may want to stop placing an emphasis on those other bindings. The Objective-C one isn’t something we support for WebKit2 anyway, so it may be a good time to not add bindings for new DOM classes; needs discussion in the project overall.

> Source/JavaScriptCore/builtins/BuiltinUtils.h:34
> +#define INITIALISE_BUILTIN_NAMES(name) , m_##name(JSC::Identifier::fromString(vm, #name)), m_##name##PrivateName(JSC::Identifier::fromUid(JSC::PrivateName(JSC::PrivateName::Description, ASCIILiteral("PrivateSymbol." #name))))

We should use the US English spelling, initialize, rather than the British English spelling, initialise, in the WebKit project. But you are just moving these; it’s strange that these were spelled the other way.

> Source/JavaScriptCore/builtins/BuiltinUtils.h:46
> +class FunctionExecutable;

Why is this declaration helpful?

> Source/JavaScriptCore/generate-js-builtins:25
> +# TODO: Move this script to another place than JavaScriptCore since it is in use for WebCore as well.

I don’t think this script needs to be moved out of JavaScriptCore. It can be a script that JavaScriptCore provides and installs where WebCore can find and use it. Think of it as part of the interface to JavaScriptCore, like one of the headers. So there may need to be a change about how we find and use it but probably not a move to another project.

> Source/WebCore/DerivedSources.make:1245
> +WEBCORE_BUILTINS = \

The comments all say "JS builtins" but the name here just says "builtins". I think that's not specific enough. Should be WEBCORE_JS_BUILTINS I think.

> Source/WebCore/bindings/js/WebCoreJSClientData.h:39
> +    WebCoreJSClientData(JSC::VM* vm)

Can this take a reference instead of a pointer to the VM? It's never null.

> Source/WebCore/bindings/js/WebCoreJSClientData.h:81
> +#if ENABLE(STREAMS_API)
> +    ReadableStreamBuiltinsWrapper& readableStreamBuiltins() { return m_readableStreamBuiltins; }
> +#endif
>  private:

Missing blank line here before the "private".

[youenn fablet]

Comment on attachment 261314 [details]
Some clean up

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

Thanks for the review!

>> Source/JavaScriptCore/builtins/BuiltinUtils.h:34
>> +#define INITIALISE_BUILTIN_NAMES(name) , m_##name(JSC::Identifier::fromString(vm, #name)), m_##name##PrivateName(JSC::Identifier::fromUid(JSC::PrivateName(JSC::PrivateName::Description, ASCIILiteral("PrivateSymbol." #name))))
> 
> We should use the US English spelling, initialize, rather than the British English spelling, initialise, in the WebKit project. But you are just moving these; it’s strange that these were spelled the other way.

OK

>> Source/JavaScriptCore/builtins/BuiltinUtils.h:46
>> +class FunctionExecutable;
> 
> Why is this declaration helpful?

It allows removing the same declaration in the generated JS builtin header file.
I'll remove it here and add it back through the generate-js-builtins script.

>> Source/JavaScriptCore/generate-js-builtins:25
>> +# TODO: Move this script to another place than JavaScriptCore since it is in use for WebCore as well.
> 
> I don’t think this script needs to be moved out of JavaScriptCore. It can be a script that JavaScriptCore provides and installs where WebCore can find and use it. Think of it as part of the interface to JavaScriptCore, like one of the headers. So there may need to be a change about how we find and use it but probably not a move to another project.

I was thinking of having a file containing code from this script that is common to both WebCore and JSC.
Then WebCore and JSC would have their own generation script reusing this common code but also adding their own specific code, like the generation of codeName##Generator(vm) implementation.
It would be convenient in WebCore to remove XXXBuiltinsWrapper.h and move its content directly in XXXBuiltins.h for instance.

>> Source/WebCore/DerivedSources.make:1245
>> +WEBCORE_BUILTINS = \
> 
> The comments all say "JS builtins" but the name here just says "builtins". I think that's not specific enough. Should be WEBCORE_JS_BUILTINS I think.

OK

>> Source/WebCore/bindings/js/WebCoreJSClientData.h:39
>> +    WebCoreJSClientData(JSC::VM* vm)
> 
> Can this take a reference instead of a pointer to the VM? It's never null.

OK.
m_readableStreamBuiltins takes a JSC::VM* as input because the macro expects vm to be a pointer.
That should be updated too at some point.

>> Source/WebCore/bindings/js/WebCoreJSClientData.h:81
>>  private:
> 
> Missing blank line here before the "private".

OK

[youenn fablet]

Created attachment 261387 [details]
Patch for landing

[youenn fablet]

Created attachment 261573 [details]
Trying to fix mac build for landing

[youenn fablet]

Created attachment 261607 [details]
Speculative win build fix

[youenn fablet]

(In reply to comment #13)
> Created attachment 261607 [details]
> Speculative win build fix

Brady, Alex, it seems Apple/Win switched to CMake.
Since that switch, I am not able to build this patch on Windows, although it works with CMake on GTK/EFL.

Any idea?

It is actually difficult for me to debug this on Windows.
Is it ok to land it as is?

[youenn fablet]

(In reply to comment #14)
> (In reply to comment #13)
> > Created attachment 261607 [details]
> > Speculative win build fix
> 
> Brady, Alex, it seems Apple/Win switched to CMake.
> Since that switch, I am not able to build this patch on Windows, although it
> works with CMake on GTK/EFL.
> 

I forgot to mention why building fails.
It seems the WebCore JS Builtins generator (ReadableStream.js -> ReadableStreamBuiltins.h/.cpp/Wrapper.h) is not kicking in on Win platform.

[Alex Christensen]

Comment on attachment 261607 [details]
Speculative win build fix

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

> Source/WebCore/CMakeLists.txt:3574
> +endforeach ()

I think you need this line inside the foreach:
list(APPEND WebCore_SOURCES
    ${DERIVED_SOURCES_WEBCORE_DIR}/${_name}Builtins.h
    ${DERIVED_SOURCES_WEBCORE_DIR}/${_name}Builtins.cpp
)
Then you shouldn't need all the ADD_SOURCE_DEPENDENCIES calls because it will generate them.

[youenn fablet]

(In reply to comment #16)
> Comment on attachment 261607 [details]
> Speculative win build fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261607&action=review
> 
> > Source/WebCore/CMakeLists.txt:3574
> > +endforeach ()
> 
> I think you need this line inside the foreach:
> list(APPEND WebCore_SOURCES
>     ${DERIVED_SOURCES_WEBCORE_DIR}/${_name}Builtins.h
>     ${DERIVED_SOURCES_WEBCORE_DIR}/${_name}Builtins.cpp
> )
> Then you shouldn't need all the ADD_SOURCE_DEPENDENCIES calls because it
> will generate them.

Downsides with that approach is that it will compile/link these files even if it is not needed to do so (STREAMS_API turned off for instance).

I will give it a try though to land this patch.
If it works out, we can fix the above issue later.

[youenn fablet]

Created attachment 261732 [details]
Changing CMakeList

[youenn fablet]

Created attachment 261741 [details]
Another try

[Alex Christensen]

(In reply to comment #17)
> Downsides with that approach is that it will compile/link these files even
> if it is not needed to do so (STREAMS_API turned off for instance).
An advantage is that it will successfully compile.  There are lots of files that we always compile that start with #if ENABLE(something).

[youenn fablet]

Created attachment 261751 [details]
Rebasing

[youenn fablet]

(In reply to comment #20)
> (In reply to comment #17)
> > Downsides with that approach is that it will compile/link these files even
> > if it is not needed to do so (STREAMS_API turned off for instance).
> An advantage is that it will successfully compile.  There are lots of files
> that we always compile that start with #if ENABLE(something).

Yes, conditional in js builtin generator like we have for IDLs might be nice.

[youenn fablet]

OK.
The builtins now kick in for Windows but including JavaScriptCore/builtins/BuiltinUtils.h in WebCore files through ForwardingHeaders is failing on Windows. 

Any idea?
Should JavaScriptCore/builtins/ path be added somewhere specifically for Windows port?

[Alex Christensen]

Add it to JavaScriptCore_FORWARDING_HEADERS_DIRECTORIES in JavaScriptCore/CMakeLists.txt

[youenn fablet]

Created attachment 261763 [details]
Fixing builtin path

[youenn fablet]

Created attachment 261766 [details]
Adding missing Inlines header for win

[WebKit Commit Bot]

Comment on attachment 261766 [details]
Adding missing Inlines header for win

Clearing flags on attachment: 261766

Committed r190155: <http://trac.webkit.org/changeset/190155>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

(In reply to comment #24)
> Add it to JavaScriptCore_FORWARDING_HEADERS_DIRECTORIES in
> JavaScriptCore/CMakeLists.txt

Patch seems to have landed properly.
Thanks for your help.

I hope that differences between the various build systems will decrease with your CMake ongoing activity.