[webkit-reviews] review denied: [Bug 179795] Add private asynchronous promise SPI. : [Attachment 327110] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 179795: Add private asynchronous promise SPI.
https://bugs.webkit.org/show_bug.cgi?id=179795

Attachment 327110: Patch

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




--- Comment #9 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 327110
  --> https://bugs.webkit.org/attachment.cgi?id=327110
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.