[webkit-reviews] review denied: [Bug 196484] Web Inspector: fake value descriptors for promises add a catch handler, preventing "rejectionhandled" events from being fired : [Attachment 366474] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 196484: Web Inspector: fake value descriptors for promises add a catch
handler, preventing "rejectionhandled" events from being fired
https://bugs.webkit.org/show_bug.cgi?id=196484

Attachment 366474: Patch

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




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 366474
  --> https://bugs.webkit.org/attachment.cgi?id=366474
Patch

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

r- just because this needs a test and we should be able to add one. Otherwise
this looks great.

> Source/JavaScriptCore/runtime/ErrorInstance.h:69
> +    bool isNativeGetterTypeError() { return m_nativeGetterTypeError; }

Nit: `const`

> Source/WebCore/Modules/streams/WritableStream.js:157
> +	   return @Promise. at reject(@makeGetterTypeError("WritableStream",
"closed"));

Awesome!

> Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:260
> +JSC::EncodedJSValue createRejectedPromiseWithTypeError(JSC::ExecState&,
const String&, bool forNativeGetter);

Nit: This boolean should really be an enum ForNativeGetter { Yes, No }. Right
now at callsites it is not easily readable.