Bug 147556

Created attachment 258051 [details] Patch using CustomBinding

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).

Created attachment 258487 [details] Adding some more tests

Created attachment 260866 [details] JS builtin within WebCore

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.

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.

(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.

Created attachment 261314 [details] Some clean up

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".

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

Created attachment 261387 [details] Patch for landing

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

Created attachment 261607 [details] Speculative win build fix

(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?

(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.

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.

(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.

Created attachment 261732 [details] Changing CMakeList

Created attachment 261741 [details] Another try

(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).

Created attachment 261751 [details] Rebasing

(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.

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?

Add it to JavaScriptCore_FORWARDING_HEADERS_DIRECTORIES in JavaScriptCore/CMakeLists.txt

Created attachment 261763 [details] Fixing builtin path

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

Comment on attachment 261766 [details] Adding missing Inlines header for win Clearing flags on attachment: 261766 Committed r190155: <http://trac.webkit.org/changeset/190155>

All reviewed patches have been landed. Closing bug.

(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.