Bug 149497 – [Streams API] Add support for JS builtins constructor

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

[youenn fablet]

To enable migrating ReadableStream to JS builtins, we need to enable constructing them using JS builtin at constructin time

[youenn fablet]

This may be done as the same time as migrating CountQueuingStrategy to JS builtins.

[youenn fablet]

Created attachment 261818 [details]
Patch

[youenn fablet]

Created attachment 261820 [details]
Rebasing IDL generated files

[Xabier Rodríguez Calvar]

Comment on attachment 261820 [details]
Rebasing IDL generated files

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

Even when the code change is more understandable with the strategy example, the patch might be a bit too big. I would split it in two different bugs with a dependency so that the review process is easier.

> Source/WebCore/ChangeLog:15
> +        * CMakeLists.txt:

Not only this, but I think the rest of the changelog could have a bit more love.

> Source/WebCore/bindings/js/JSDOMBinding.cpp:663
> +    auto* thisObject = jsCast<DOMConstructorJSBuiltinObject*>(cell);

Do you need the auto*? Isn't it enough with auto?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4793
> +            push(@$outputArray, "    auto* castedThis = jsCast<${constructorClassName}*>(state->callee());\n");

Same with auto and *

[youenn fablet]

> > Source/WebCore/ChangeLog:15
> > +        * CMakeLists.txt:
> 
> Not only this, but I think the rest of the changelog could have a bit more
> love.

Right, will do in next patch or at landing time.

> 
> > Source/WebCore/bindings/js/JSDOMBinding.cpp:663
> > +    auto* thisObject = jsCast<DOMConstructorJSBuiltinObject*>(cell);
> 
> Do you need the auto*? Isn't it enough with auto?

Looking at the coding style document and in JSDOMBinding.cpp, examples use 'auto&'. Lets' see what others will say about this.

[Darin Adler]

Comment on attachment 261820 [details]
Rebasing IDL generated files

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

Where is the test coverage for the use of JSBuiltinConstructor in CountQueingStrategy? I see lots of new code but not test showing the code is doing its job. Maybe there is an existing test that keeps working?

> Source/WebCore/Modules/streams/CountQueuingStrategy.h:42
> +    static RefPtr<CountQueuingStrategy> create() { return adoptRef(*new CountQueuingStrategy()); }

Return value here should be Ref, not RefPtr.

No need for the () after the type name.

Typically the point of making a create function is to make sure nobody constructs the class directly by accident, getting the reference counting wrong. That means we normally make the constructor private. That is not done here and probably should be done. Just "CountQueuingStrategy() -> default;" after "private".

This class is really confusing if you don’t understand that all the implementation is in JavaScript. We may want to figure out how to document this sort of thing as we do future work. Or even come up with a way where we don’t need to create this boilerplate if there is no interesting C++ code.

> Source/WebCore/bindings/js/JSDOMBinding.cpp:649
> +void callInitializeFunction(JSC::ExecState& state, JSC::JSValue object, JSC::JSFunction* function)

Is there no other place we can share this code with? It seems like a lot to write out doing almost nothing interesting.

> Source/WebCore/bindings/js/JSDOMBinding.cpp:657
> +        arguments.append(state.argument(i));

Should be using uncheckedArgument here.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:663
>> +    auto* thisObject = jsCast<DOMConstructorJSBuiltinObject*>(cell);
> 
> Do you need the auto*? Isn't it enough with auto?

We often write auto* rather than auto in a case like this to emphasize that it’s a pointer type, since it takes only one character to do that. The use of auto& at other call sites is specifically to avoid unnecessarily copying an object, easy to do by accident if you use auto in a modern for loop for example. So another purpose of auto* is to make it clear that we definitely are not copying an object, just a pointer. So auto would be fine, but auto* also seems fine.

> Source/WebCore/bindings/js/JSDOMBinding.h:88
> +void callInitializeFunction(JSC::ExecState&, JSC::JSValue, JSC::JSFunction*);

Maybe the initializeFunction could be a reference rather than a pointer even if other existing code uses a pointer for this.

It’s not really clear what the JSValue is. Might need a name for that argument. Maybe even change it to JSObject?

> Source/WebCore/bindings/js/JSDOMBinding.h:90
> +template<typename WrapperClass, typename DOMClass> inline JSC::EncodedJSValue createFromJSBuiltin(JSC::ExecState& state, JSC::JSFunction* initializeFunction, JSDOMGlobalObject& globalObject)

Maybe initializeFunction could be a reference rather than a pointer even if other existing code uses a pointer for this.

> Source/WebCore/bindings/js/JSDOMBinding.h:92
> +    JSC::JSValue object = toJS(&state, &globalObject, DOMClass::create());

If this is always an object, then JSObject& might be a better type than JSValue.

> Source/WebCore/bindings/js/JSDOMBinding.h:120
> +    DOMConstructorJSBuiltinObject(JSC::Structure* structure, JSDOMGlobalObject* globalObject)

Maybe both of these could be reference instead of pointers even if existing classes use references.

> Source/WebCore/bindings/js/JSDOMBinding.h:123
> +    static void visitChildren(JSC::JSCell*, JSC::SlotVisitor&);

Maybe this should take a JSCell& rather than JSCell* even if other classes use pointers.

> Source/WebCore/bindings/js/JSDOMBinding.h:125
> +    JSC::WriteBarrier<JSC::JSFunction> m_initializeFunction;

Can this be private rather than protected please?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4970
> +    if ($interface->extendedAttributes->{"JSBuiltinConstructor"}) {
> +        my $initializeFunctionName = GetJSBuiltinFunctionName(${className}, "initialize" . ${interfaceName});
> +        push(@$outputArray, "    m_initializeFunction.set(vm, this, JSC::JSFunction::createBuiltinFunction(vm, ${initializeFunctionName}(vm), globalObject));\n");
> +    }
> +
> +
>      push(@$outputArray, "}\n\n");

Extra blank line here. We only need one.

[youenn fablet]

Thanks for the review.

(In reply to comment #6)
> Comment on attachment 261820 [details]
> Rebasing IDL generated files
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261820&action=review
> 
> Where is the test coverage for the use of JSBuiltinConstructor in
> CountQueingStrategy? I see lots of new code but not test showing the code is
> doing its job. Maybe there is an existing test that keeps working?

This patch is replacing C++ CountQueuingStrategy with JS builtin CountQueuingStrategy.
Existing tests instantiating and using CQS can be found in LayoutTests/streams/reference-implementation, mostly in count-queuing-strategy.html.


> > Source/WebCore/Modules/streams/CountQueuingStrategy.h:42
> > +    static RefPtr<CountQueuingStrategy> create() { return adoptRef(*new CountQueuingStrategy()); }
> 
> Return value here should be Ref, not RefPtr.
> 
> No need for the () after the type name.
> 
> Typically the point of making a create function is to make sure nobody
> constructs the class directly by accident, getting the reference counting
> wrong. That means we normally make the constructor private. That is not done
> here and probably should be done. Just "CountQueuingStrategy() -> default;"
> after "private".
> 
> This class is really confusing if you don’t understand that all the
> implementation is in JavaScript. We may want to figure out how to document
> this sort of thing as we do future work. Or even come up with a way where we
> don’t need to create this boilerplate if there is no interesting C++ code.

Yes, I plan to improve that in another patch.
One option is to inline the boilerplate code in JSXX when JSBuiltinConstructor is in action.
Another cleaner option might be to remove the need for having a XX DOM class for JSXX in that case.

> 
> > Source/WebCore/bindings/js/JSDOMBinding.cpp:649
> > +void callInitializeFunction(JSC::ExecState& state, JSC::JSValue object, JSC::JSFunction* function)
> 
> Is there no other place we can share this code with? It seems like a lot to
> write out doing almost nothing interesting.

Not that I am aware.
I will rename the function to something like callFunctionWithCurrentArguments.

> 
> > Source/WebCore/bindings/js/JSDOMBinding.cpp:657
> > +        arguments.append(state.argument(i));
> 
> Should be using uncheckedArgument here.

OK

> >> Source/WebCore/bindings/js/JSDOMBinding.cpp:663
> >> +    auto* thisObject = jsCast<DOMConstructorJSBuiltinObject*>(cell);
> > 
> > Do you need the auto*? Isn't it enough with auto?
> 
> We often write auto* rather than auto in a case like this to emphasize that
> it’s a pointer type, since it takes only one character to do that. The use
> of auto& at other call sites is specifically to avoid unnecessarily copying
> an object, easy to do by accident if you use auto in a modern for loop for
> example. So another purpose of auto* is to make it clear that we definitely
> are not copying an object, just a pointer. So auto would be fine, but auto*
> also seems fine.
> 
> > Source/WebCore/bindings/js/JSDOMBinding.h:88
> > +void callInitializeFunction(JSC::ExecState&, JSC::JSValue, JSC::JSFunction*);
> 
> Maybe the initializeFunction could be a reference rather than a pointer even
> if other existing code uses a pointer for this.

OK

> It’s not really clear what the JSValue is. Might need a name for that
> argument. Maybe even change it to JSObject?

OK, let's change it to JSObject, and name it thisObject

> > Source/WebCore/bindings/js/JSDOMBinding.h:90
> > +template<typename WrapperClass, typename DOMClass> inline JSC::EncodedJSValue createFromJSBuiltin(JSC::ExecState& state, JSC::JSFunction* initializeFunction, JSDOMGlobalObject& globalObject)
> 
> Maybe initializeFunction could be a reference rather than a pointer even if
> other existing code uses a pointer for this.

OK

> > Source/WebCore/bindings/js/JSDOMBinding.h:92
> > +    JSC::JSValue object = toJS(&state, &globalObject, DOMClass::create());
> 
> If this is always an object, then JSObject& might be a better type than
> JSValue.

OK

> > Source/WebCore/bindings/js/JSDOMBinding.h:120
> > +    DOMConstructorJSBuiltinObject(JSC::Structure* structure, JSDOMGlobalObject* globalObject)
> 
> Maybe both of these could be reference instead of pointers even if existing
> classes use references.

This is code used from generated code and that would require changing other classes.
This should be put forward as a follow-up patch.

> > Source/WebCore/bindings/js/JSDOMBinding.h:123
> > +    static void visitChildren(JSC::JSCell*, JSC::SlotVisitor&);
> 
> Maybe this should take a JSCell& rather than JSCell* even if other classes
> use pointers.

This is code used from generated code and that would require changing other classes.
This should be put forward as a follow-up patch.

> > Source/WebCore/bindings/js/JSDOMBinding.h:125
> > +    JSC::WriteBarrier<JSC::JSFunction> m_initializeFunction;
> 
> Can this be private rather than protected please?

OK, will add protected getter/setter.

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4970
> > +    if ($interface->extendedAttributes->{"JSBuiltinConstructor"}) {
> > +        my $initializeFunctionName = GetJSBuiltinFunctionName(${className}, "initialize" . ${interfaceName});
> > +        push(@$outputArray, "    m_initializeFunction.set(vm, this, JSC::JSFunction::createBuiltinFunction(vm, ${initializeFunctionName}(vm), globalObject));\n");
> > +    }
> > +
> > +
> >      push(@$outputArray, "}\n\n");
> 
> Extra blank line here. We only need one.

OK

[youenn fablet]

Created attachment 261863 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 261863 [details]
Patch for landing

Clearing flags on attachment: 261863

Committed r190198: <http://trac.webkit.org/changeset/190198>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.