Bug 146060 – Bindings generator should generate code to catch exception and reject promises for Promise-based APIs

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

[youenn fablet]

A promise returning function is not expected to throw exception.
In case of argument mismatch or other cases, the exceptions should be transmitted to the application by rejecting the promise.
The only case the function should throw is if the function is called on a this value which does not have the right type.
The binding generator should handle these cases properly.

[youenn fablet]

Created attachment 255017 [details]
Patch

[Darin Adler]

Comment on attachment 255017 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMPromise.h:41
> +    static void rejectPromiseWithException(JSC::ExecState*, JSDOMGlobalObject*, JSC::JSPromiseDeferred*);

I think all the arguments should be references.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2799
> +                push(@implContent, "static inline EncodedJSValue ${functionName}Promise(ExecState*, " . $className . "*, JSPromiseDeferred*);\n");
> +                push(@implContent, "EncodedJSValue JSC_HOST_CALL ${functionName}(ExecState* exec)\n");
> +                push(@implContent, "{\n");
> +
> +                GenerateFunctionCastedThis($interface, $interfaceName, $className, $function);
> +
> +                push(@implContent, "    JSPromiseDeferred* promiseDeferred = JSPromiseDeferred::create(exec, castedThis->globalObject());\n");
> +                push(@implContent, "    ${functionName}Promise(exec, castedThis, promiseDeferred);\n");
> +
> +                push(@implContent, "    if (exec->hadException())\n");
> +                push(@implContent, "        DeferredWrapper::rejectPromiseWithException(exec, castedThis->globalObject(), promiseDeferred);\n");
> +                push(@implContent, "    ASSERT(!exec->hadException());\n");
> +                push(@implContent, "    return JSValue::encode(promiseDeferred->promise());\n");
> +                push(@implContent, "}\n");

Normally we try to make the generated code as small as possible, using normal C++ programming for everything we can. We’ve strayed from that a bit, but it’s important to not load the total amount of code this generates. Can we use a helper function or function template to make he actual generated code as small as possible? It seems that once we call xxxPromise we do nothing more that depends on the arguments. I’d like to see this entire thing be a one line invocation of a function template that is in JSDOMPromise.h.

[youenn fablet]

Created attachment 255109 [details]
Patch for landing

[youenn fablet]

(In reply to comment #2)
> Comment on attachment 255017 [details]
> Patch
> 
> nothing more that depends on the arguments. I’d like to see this entire
> thing be a one line invocation of a function template that is in
> JSDOMPromise.h.

Thanks for the idea.
Latest patch tries to integrate some of this.
Let me know if you would like further adjustment.
If not, I will cq+ it tomorrow.

[WebKit Commit Bot]

Comment on attachment 255109 [details]
Patch for landing

Clearing flags on attachment: 255109

Committed r185739: <http://trac.webkit.org/changeset/185739>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 255109 [details]
Patch for landing

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

Looks really good.

> Source/WebCore/bindings/js/JSDOMPromise.h:62
> +template<class JSClassName>

The type here is the JavaScript class, not the JavaScript class name. I suggest the name WrapperClass here instead of JSClassName. Also, we almost always use typename rather than class in templates, although I’m not entirely sure why.

> Source/WebCore/bindings/js/JSDOMPromise.h:63
> +inline JSC::JSValue callPromiseFunction(JSC::ExecState& state, JSClassName& jsObject, JSC::EncodedJSValue promiseFunction(JSC::ExecState*, JSClassName*, JSC::JSPromiseDeferred*))

A typical name used in bindings for what we are calling jsObject here is “wrapper”