Bug 213684 – Improve error message for primitive callback interfaces

• Component: Bindings
• Status: RESOLVED FIXED

[Alexey Shvayka]

Improve error message for primitive callback interfaces

[Alexey Shvayka]

Created attachment 402969 [details]
Patch

[Alexey Shvayka]

Created attachment 402970 [details]
Patch

Reset bindings tests results.

[Darin Adler]

Comment on attachment 402970 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:218
> +    appendArgumentMustBe(builder, argumentIndex, argumentName, interfaceName, functionName);

At some point should refactor this to use variadic append instead of StringBuilder, since there’s no reason not to use that more efficient approach.

[Alexey Shvayka]

(In reply to Darin Adler from comment #3)
> > Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:218
> > +    appendArgumentMustBe(builder, argumentIndex, argumentName, interfaceName, functionName);
> 
> At some point should refactor this to use variadic append instead of
> StringBuilder, since there’s no reason not to use that more efficient
> approach.

Thank you for review, Darin.

I'm not sure I've got "instead of StringBuilder" bit.
Would you prefer appendArgumentMustBe() written similar to refactors in r261437?

```
builder.append(
    "Argument ",
    argumentIndex + 1,
    " ('",
    argumentName,
    "') to ",
    functionName ? interfaceName : "the ",
    functionName ? '.' : interfaceName,
    functionName ? functionName : " constructor",
    " must be ",
);
```

[Darin Adler]

(In reply to Alexey Shvayka from comment #4)
> (In reply to Darin Adler from comment #3)
> > > Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:218
> > > +    appendArgumentMustBe(builder, argumentIndex, argumentName, interfaceName, functionName);
> > 
> > At some point should refactor this to use variadic append instead of
> > StringBuilder, since there’s no reason not to use that more efficient
> > approach.
> 
> Thank you for review, Darin.
> 
> I'm not sure I've got "instead of StringBuilder" bit.

Instead of appendArgumentMustBe I was suggesting writing a function throwArgumentMustBe like this:

template<typename... StringTypes> static JSC::EncodedJSValue throwArgumentMustBe(JSC::JSGlobalObject& globalObject, JSC::ThrowScope& scope, unsigned argumentIndex, const char* argumentName, const char* interfaceName, const char* functionName, StringTypes ...strings)
{
    auto prefix = functionName ? "" : "the ";
    auto suffix = functionName ? "." : "constructor";
    auto optionalFunctionName = functionName ? functionName : "";
    return throwVMTypeError(&globalObject, scope, makeString(
        "Argument ", argumentIndex + 1, " ('", argumentName, "') to ",
        prefix, interfaceName, suffix, optionalFunctionName, " must be ", strings...
    ));
}

And then callers would have this:

    return throwArgumentMustBe(lexicalGlobalObject, script, argumentIndex, argumentName,
        functionInterfaceName, functionName, "one of: ", expectedValues);

    return throwArgumentMustBe(lexicalGlobalObject, script, argumentIndex, argumentName,
        functionInterfaceName, functionName, "a function");

    return throwArgumentMustBe(lexicalGlobalObject, script, argumentIndex, argumentName,
        functionInterfaceName, functionName, "an object");

It comes out kind of nice, don’t you think? Would be even better if all those arguments weren’t separate and there was some kind of struct or object they were contained in, but good enough for now.

On reflection, the current uses are simple enough that it could be done without a template:

static JSC::EncodedJSValue throwArgumentMustBe(JSC::JSGlobalObject& globalObject, JSC::ThrowScope& scope, unsigned argumentIndex, const char* argumentName, const char* interfaceName, const char* functionName, const char* firstSuffix, const char* secondSuffix = "")
{
    ...
}

And still called the same way.

This comes out of just three things:

1) Recognition that makeString is more efficient than StringBuilder, StringBuilder to be avoided if possible.
2) Refactoring thinking: how can we maximize shared code while maintaining a high degree of clarity?
3) Realization that lists of arbitrary string-ifiable things can be passed around with parameter packs.

[Alexey Shvayka]

Comment on attachment 402970 [details]
Patch

(In reply to Darin Adler from comment #5)
> Instead of appendArgumentMustBe I was suggesting writing a function
> throwArgumentMustBe like this:

I appreciate the insight and detailed explanation.

> It comes out kind of nice, don’t you think?

Absolutely!
I'll submit a follow-up with proposed refactor and also remove StringBuilder from throwRequiredMemberTypeError().

[EWS]

Committed r263638: <https://trac.webkit.org/changeset/263638>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402970 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64868672>