Bug 206364

Created attachment 387938 [details]
Patch

Comment on attachment 387938 [details]
Patch

r=me

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.

(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"

Comment on attachment 387938 [details]
Patch

Clearing flags on attachment: 387938

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

All reviewed patches have been landed.  Closing bug.

(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.

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.