Bug 179795 – Add private asynchronous promise SPI.

• Status: NEW
• Component: JavaScriptCore

[Keith Miller]

Add private asynchronous promise SPI.

[Keith Miller]

Created attachment 327107 [details]
Patch

[Keith Miller]

Created attachment 327108 [details]
Patch

[EWS Watchlist]

Attachment 327108 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:51:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:60:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:60:  The parameter name "asynchronousPromiseToken" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:70:  The parameter name "asynchronousPromiseToken" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 327110 [details]
Patch

[EWS Watchlist]

Attachment 327110 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:51:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:60:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:60:  The parameter name "asynchronousPromiseToken" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:70:  The parameter name "asynchronousPromiseToken" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 327110 [details]
Patch

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

> Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.cpp:58
> +    return toRef(promise);

Can we just make this return promise->promise()? We shouldn't expose JSPromiseDeffered*, even in an initial SPI IMO. This will come back to bite us.
Maybe we can augment the API to allow to be mapped via promise, and then create a map from JSObject* -> JSPromiseDeferred*. That way we can internal map from the return value here to JSPromiseDeferred*.

[Saam Barati]

Comment on attachment 327110 [details]
Patch

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

> Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.cpp:90
> +        if (resolution == PromiseResolved)
> +            promise->resolve(exec, toJS(exec, result));
> +        else
> +            promise->reject(exec, toJS(exec, result));

We need to grab the lock here? Or is already held?

[Saam Barati]

Comment on attachment 327110 [details]
Patch

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

> Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:49
> + @discussion Unlike the JSAsynchronousPromiseScheduleResolveSoon function, JSAsynchronousPromiseMakeInContext cannot be called concurrently with JS execution. The promise object underlying the token will be retained  until JSAsynchronousPromiseScheduleResolveSoon is called.

I would perhaps state this slightly differently:
"cannot be called concurrently with JS execution"
=>
"must be called from the JS execution thread"

> Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:64
> + @abstract Notifies the JSContextGroup of promise it can resolve the promise on the runloop the JSContextGroup was created on.

"Notifies the JSContextGroup of promise it can resolve the promise on the runloop the JSContextGroup was created on" => "Notifies the JSContextGroup of a promise it can resolve on the runloop the JSContextGroup was created on"

[Geoffrey Garen]

Comment on attachment 327110 [details]
Patch

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

I don't think we want to publish half-baked APIs.

If someone wants to do some prototyping, they can build JavaScriptCore for themselves and use the code you've written. For APIs we check into the open source project and the Apple framework, we want code we believe in.

> Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:36
> +// This API is intended for prototyping purposes and is likely to change in the future.

What kinds of change are we considering? Why not just implement the change now?

Why are we making new APIs in C instead of ObjC?

> Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:38
> +enum PromiseResolution {

Needs name spacing like JSPromiseResolution.

> Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:43
> +typedef struct JSOpaqueAsynchronousPromiseToken* JSAsynchronousPromiseToken;

We should say "promise" everywhere instead of "asynchronous promise" because promises are asynchronous by nature and we should not rename language concepts when making them API.

> Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:51
> +JS_EXPORT JSAsynchronousPromiseToken JSAsynchronousPromiseMakeInContext(JSContextRef context);

Is a promise a JavaScript object? If so, these should be JSObject functions, like JSObjectMakePromise.

> Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:60
> +JS_EXPORT JSObjectRef JSAsynchronousPromiseGetPromiseObject(JSContextRef context, JSAsynchronousPromiseToken asynchronousPromiseToken);

This needs a better term than "promise object". It is not specific enough to ask a "promise" to return a "promise object". The distinction isn't clear. In JavaScript, you pass an executor function to the promise constructor. Is that what this API returns?

> Source/JavaScriptCore/API/JSAsynchronousPromisePrivate.h:70
> +JS_EXPORT bool JSAsynchronousPromiseScheduleResolveSoon(JSAsynchronousPromiseToken asynchronousPromiseToken, PromiseResolution (^resolutionCallback)(JSGlobalContextRef context, JSValueRef* result));

I would just call this "resolve" rather than "schedule resolve soon". Again, all promise resolution is intrinsically async.

> Source/JavaScriptCore/JSAsynchronousPromiseTests.mm:46
> +        JSAsynchronousPromiseToken token = JSAsynchronousPromiseMakeInContext([context JSGlobalContextRef]);

It's weird that "PromiseMake" returns a "PromiseToken". It should just return a Promise. Or it should be called "make promise token". But why a token? In JS, when you make a promise you get a promise, not a promise token.