Bug 172481 – [WebIDL] Overloaded functions unnecessarily duplicate argument checks

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Sam Weinig]

Overloaded functions are currently unnecessarily duplicate argument checks. For example, let's take CanvasRenderingContext2d's stroke function. It has two overloads:

    void stroke(DOMPath path);
    void stroke();

This generates the main bindings function:

EncodedJSValue JSC_HOST_CALL jsCanvasRenderingContext2DPrototypeFunctionStroke(ExecState* state)
{
    VM& vm = state->vm();
    auto throwScope = DECLARE_THROW_SCOPE(vm);
    UNUSED_PARAM(throwScope);
    size_t argsCount = std::min<size_t>(1, state->argumentCount());
    if (argsCount == 0) {
        return jsCanvasRenderingContext2DPrototypeFunctionStroke2(state);
    }
    if (argsCount == 1) {
        return jsCanvasRenderingContext2DPrototypeFunctionStroke1(state);
    }
    return throwVMTypeError(state, throwScope);
}

and then two the helpers, jsCanvasRenderingContext2DPrototypeFunctionStroke1 and jsCanvasRenderingContext2DPrototypeFunctionStroke2.

Taking a look at jsCanvasRenderingContext2DPrototypeFunctionStroke1, we see the argumentCount is once again checked, despite it already having been done in the main function: 

static inline JSC::EncodedJSValue jsCanvasRenderingContext2DPrototypeFunctionStroke1Caller(JSC::ExecState* state, JSCanvasRenderingContext2D* castedThis, JSC::ThrowScope& throwScope)
{
    UNUSED_PARAM(state);
    UNUSED_PARAM(throwScope);
    auto& impl = castedThis->wrapped();
    if (UNLIKELY(state->argumentCount() < 1))
        return throwVMError(state, throwScope, createNotEnoughArgumentsError(state));
    auto path = convert<IDLInterface<DOMPath>>(*state, state->uncheckedArgument(0), [](JSC::ExecState& state, JSC::ThrowScope& scope) { throwArgumentTypeError(state, scope, 0, "path", "CanvasRenderingContext2D", "stroke", "DOMPath"); });
    RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
    impl.stroke(*path);
    return JSValue::encode(jsUndefined());
}


We could further improve things by taking advantage of the cases where disambiguation takes place, and retain the type information into the helpers.

[Sam Weinig]

Created attachment 311093 [details]
Patch

[Chris Dumez]

Comment on attachment 311093 [details]
Patch

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

r=me with nit.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4056
> +        foreach my $function (@functions) {

FYI, I'd personally if we used IDL naming consistently in the bindings generator. i.e. function -> operation.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4746
> +    return if $function->{overloads} && @{$function->{overloads}} > 1;

Maybe a comment to explain why we do not need argument count check for overloads?

[Sam Weinig]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 311093 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311093&action=review
> 
> r=me with nit.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4056
> > +        foreach my $function (@functions) {
> 
> FYI, I'd personally if we used IDL naming consistently in the bindings
> generator. i.e. function -> operation.

I'll do this in another pass. Get them all at once.

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4746
> > +    return if $function->{overloads} && @{$function->{overloads}} > 1;
> 
> Maybe a comment to explain why we do not need argument count check for
> overloads?

Will do.

[Sam Weinig]

Committed r217369: <http://trac.webkit.org/changeset/217369>

[Radar WebKit Bug Importer]

<rdar://problem/32479792>