[webkit-reviews] review granted: [Bug 166752] Handle IDLPromise<> properly : [Attachment 309098] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 166752: Handle IDLPromise<> properly
https://bugs.webkit.org/show_bug.cgi?id=166752

Attachment 309098: Patch

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




--- Comment #31 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 309098
  --> https://bugs.webkit.org/attachment.cgi?id=309098
Patch

OK, let's go!
Some nits below.

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

> Source/JavaScriptCore/runtime/JSPromise.cpp:99
> +    auto scope = DECLARE_THROW_SCOPE(vm);

maybe use auto for all 3 and below as well?

> Source/JavaScriptCore/runtime/JSPromise.h:55
> +    JS_EXPORT_PRIVATE static JSPromise* resolve(JSGlobalObject*, JSValue);

Shouldn't we try to pass a JSGlobalObject&?

> Source/WebCore/bindings/js/JSDOMConvertPromise.h:43
> +	   JSDOMGlobalObject* globalObject =
jsDynamicDowncast<JSDOMGlobalObject*>(vm, state.lexicalGlobalObject());

auto*

> Source/WebCore/bindings/js/JSDOMConvertPromise.h:49
> +	   JSC::JSPromise* promise = JSC::JSPromise::resolve(globalObject,
value);

Ditto

> Source/WebCore/dom/RejectedPromiseTracker.cpp:53
>  class RejectedPromise {

Do we still need RejectedPromise?

> Source/WebCore/dom/RejectedPromiseTracker.cpp:125
> +void RejectedPromiseTracker::promiseHandled(ExecState&, JSDOMGlobalObject&
globalObject, JSPromise& promise)

Change signature in a follow-up?

> Source/WebCore/dom/RejectedPromiseTracker.cpp:167
> +	   Ref<DOMPromise> domPromise = unhandledPromise.promise();

Why do we need to take a Ref here?

> Source/WebCore/dom/RejectedPromiseTracker.cpp:170
> +	   ExecState& state = *domPromise->globalObject()->globalExec();

auto?

> Source/WebCore/dom/RejectedPromiseTracker.cpp:200
> +    Ref<DOMPromise> domPromise = rejectedPromise.promise();

Why taking a ref here?

> Source/WebCore/dom/RejectedPromiseTracker.cpp:209
> +    initializer.reason = promise.result(vm);

Brace initializer maybe?