[webkit-reviews] review granted: [Bug 175246] [WebIDL] Add support for Promise<> attributes : [Attachment 317392] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 175246: [WebIDL] Add support for Promise<> attributes
https://bugs.webkit.org/show_bug.cgi?id=175246

Attachment 317392: Patch

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




--- Comment #21 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 317392
  --> https://bugs.webkit.org/attachment.cgi?id=317392
Patch

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

r=me

> Source/WebCore/ChangeLog:70
> +	       cycles when the resolved value is the owner such as is the case
with FontFace and FontFaceSet.

OK, nice. I think noting this in DOMPromiseProxyWithResolveCallback code is
nice.

> Source/WebCore/bindings/js/DOMPromiseProxy.h:42
> +    using ValueOrException = Variant<Value, Exception>;

How about using WTF::Expected<Value, Exception> instead?

> Source/WebCore/bindings/js/DOMPromiseProxy.h:66
> +    using ValueOrException = Variant<Value, Exception>;

Ditto.

> Source/WebCore/bindings/js/DOMPromiseProxy.h:89
> +    using ValueOrException = Variant<Value, Exception>;

Ditto.

> Source/WebCore/bindings/js/DOMPromiseProxy.h:129
> +    // DeferredPromise can fail construction during worker abrupt
termination.
> +    auto deferredPromise = DeferredPromise::create(state, globalObject,
DeferredPromise::Mode::RetainPromiseOnResolve);

Great! I think we need to review the other DeferredPromise construction
(JSPromiseDeferred::create use) as well (in a separate patch).

> Source/WebCore/bindings/js/DOMPromiseProxy.h:146
> +inline void DOMPromiseProxy<IDLType>::reset()

If it does not take any argument, I think the name `clear` is better for that.
When using `reset`, I expect that `reset` method takes an argument to be set.

> Source/WebCore/bindings/js/DOMPromiseProxy.h:215
> +inline void DOMPromiseProxy<IDLVoid>::reset()

Ditto.

> Source/WebCore/bindings/js/DOMPromiseProxy.h:283
> +inline void DOMPromiseProxyWithResolveCallback<IDLType>::reset()

Ditto.