[webkit-reviews] review granted: [Bug 189809] Add Promise SPI : [Attachment 350382] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 189809: Add Promise SPI
https://bugs.webkit.org/show_bug.cgi?id=189809

Attachment 350382: Patch

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




--- Comment #12 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 350382
  --> https://bugs.webkit.org/attachment.cgi?id=350382
Patch

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

r=me with comments

> Source/JavaScriptCore/ChangeLog:15
> +	   in/to that callback the promise is automagically rejected. The

I don't know what you mean by "to that callback" here

> Source/JavaScriptCore/ChangeLog:16
> +	   other methods create an pre-resolved or rejected promise as this

an => a

> Source/JavaScriptCore/ChangeLog:17
> +	   appears to be the most common way to initialize a promise.

"the most common" => "a common"

> Source/JavaScriptCore/ChangeLog:24
> +	   corrupt state if an Obj-C exception unwinds JS frames.

unwinds => "unwinds through" or "unwinds past"

> Source/JavaScriptCore/API/JSObjectRef.cpp:294
> +    JSPromiseDeferred::DeferredData data =
JSPromiseDeferred::createDeferredData(exec, globalObject,
globalObject->promiseConstructor());
> +    handleExceptionIfNeeded(scope, exec, nullptr);

The semantics here seem weird, or at least non-obvious to me, if this actually
throws. I think we should set the passed in exception if this throws.
Otherwise, it's pretty weird that the code below just stores null.

> Source/JavaScriptCore/API/JSValue.mm:163
> +    JSObjectRef promise = JSObjectMakeDeferredPromise([context
JSGlobalContextRef], &resolve, &reject, &exception);

is it worth stating this can throw in the documentation of the SPI? When does
this actually throw?

> Source/JavaScriptCore/API/JSValue.mm:180
> +	   [rejection callWithArguments:@[context.exception]];

Does this automatically clear the exception? I don't think it does. Do we want
to clear it?

> Source/JavaScriptCore/API/JSValuePrivate.h:38
> + being initialized. The resolve and reject parameters are functions that
> + can be called to notify any dependent promises about the state of the
> + new promise JSValue.

This explanation feels weird to me. Maybe we can state it less in terms of
promises down the chain, and just in terms of resolve/reject?

> Source/JavaScriptCore/API/JSValuePrivate.h:42
> + the resolve and reject callbacks each normally take a single result value,
which they

"result value" seems weird in the context of reject. Maybe just say "single
value"?

> Source/JavaScriptCore/API/JSValuePrivate.h:57
> ++ (JSValue *)valueWithNewResolvedPromiseWithResult:(id)result
inContext:(JSContext *)context JSC_API_AVAILABLE(macosx(JSC_MAC_TBA),
ios(JSC_IOS_TBA));

nit: This name feels overly verbose to me. Maybe:
valueWithNewPromiseResolvedWith

> Source/JavaScriptCore/API/JSValuePrivate.h:62
> + @param reason The result value to be passed to any reactions.

ditto about "result" value here. Seems like we need some other way to state
failure other than "result". How does TC39 distinguish between them in naming?

> Source/JavaScriptCore/API/JSValuePrivate.h:67
> ++ (JSValue *)valueWithNewRejectedPromiseWithReason:(id)reason
inContext:(JSContext *)context JSC_API_AVAILABLE(macosx(JSC_MAC_TBA),
ios(JSC_IOS_TBA));

ditto

> Source/JavaScriptCore/API/tests/testapi.c:1982
> +    const char *scriptPath = "testapi.js";

"const char *" => "const char* "

> Source/JavaScriptCore/API/tests/testapi.cpp:88
> +    operator JSC::ExecState*() { return toJS(m_context); }

👌

> Source/JavaScriptCore/API/tests/testapi.mm:1729
> +	   checkResult(@"Exception set in callback should not propegate",
!context.exception);

propegate => propagate

> Source/JavaScriptCore/API/tests/testapi.mm:1739
> +	   checkResult(@"Exception thrown in callback should not propegate",
!context.exception);

propegate => propagate

> Source/JavaScriptCore/runtime/JSPromiseDeferred.h:48
> +	   WTF_FORBID_HEAP_ALLOCATION;

style nit: Put this at the top

> Source/WTF/wtf/mac/MainThreadMac.mm:65
> +static bool mainThreadEstablishedAsPthreadMain { false };
> +static pthread_t mainThreadPthread { nullptr };
> +static NSThread* mainThreadNSThread { nullptr };

Though I like this style more, I think WK style does say we don't do this for
static variables.