Bug 206364 – Make the callAsyncJavaScriptFunction function actually be async (so await works)

• Component: WebKit Website
• Status: RESOLVED FIXED

[Brady Eidson]

Make the callAsyncJavaScriptFunction function actually be async (so await works)

<rdar://problem/58571682>

[Brady Eidson]

Created attachment 387938 [details]
Patch

[Geoffrey Garen]

Comment on attachment 387938 [details]
Patch

r=me

[Devin Rousso]

Comment on attachment 387938 [details]
Patch

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

> Source/WebCore/bindings/js/ScriptController.cpp:607
> +    functionStringBuilder.append("(async function(");

One issue with making a function `async` is that they _always_ return a `Promise`.  I think it would be better to have `ScriptController:callInWorld` take an `enum class IsAsync { Yes, No };` so that callers can decide whether or not they want this.

[Brady Eidson]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 387938 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387938&action=review
> 
> > Source/WebCore/bindings/js/ScriptController.cpp:607
> > +    functionStringBuilder.append("(async function(");
> 
> One issue with making a function `async` is that they _always_ return a
> `Promise`.  

Which is fine, because literally the only client of this is "callAsyncJavaScriptFunction" which expects promise results.

> I think it would be better to have
> `ScriptController:callInWorld` take an `enum class IsAsync { Yes, No };` so
> that callers can decide whether or not they want this.

There's no reason to do now that because there would be no client of "No"

[WebKit Commit Bot]

Comment on attachment 387938 [details]
Patch

Clearing flags on attachment: 387938

Committed r254704: <https://trac.webkit.org/changeset/254704>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 387938 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387938&action=review
> 
> > Source/WebCore/bindings/js/ScriptController.cpp:607
> > +    functionStringBuilder.append("(async function(");
> 
> One issue with making a function `async` is that they _always_ return a
> `Promise`.  I think it would be better to have
> `ScriptController:callInWorld` take an `enum class IsAsync { Yes, No };` so
> that callers can decide whether or not they want this.

Why would the API change here? When the Promise that’s returned is resolved, we IPC the result back to the UI process, so I think it should not be (very) observable to users. (Like if you treat it like a non async function, and do something like “return 42;”, everything Just Works and the UI process is called back with “42”.)

I think the only concern here is that async functions have slightly different semantics than non async functions. I think(?) things like reserved identifiers and such. But I think that’s ok here, especially because this will be documented.

[Devin Rousso]

Comment on attachment 387938 [details]
Patch

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

>>>> Source/WebCore/bindings/js/ScriptController.cpp:607
>>>> +    functionStringBuilder.append("(async function(");
>>> 
>>> One issue with making a function `async` is that they _always_ return a `Promise`.  I think it would be better to have `ScriptController:callInWorld` take an `enum class IsAsync { Yes, No };` so that callers can decide whether or not they want this.
>> 
>> Which is fine, because literally the only client of this is "callAsyncJavaScriptFunction" which expects promise results.
> 
> Why would the API change here? When the Promise that’s returned is resolved, we IPC the result back to the UI process, so I think it should not be (very) observable to users. (Like if you treat it like a non async function, and do something like “return 42;”, everything Just Works and the UI process is called back with “42”.)
> 
> I think the only concern here is that async functions have slightly different semantics than non async functions. I think(?) things like reserved identifiers and such. But I think that’s ok here, especially because this will be documented.

I misread how `ScriptController::callInWorld` was being used.  My apologies for the confusion.