[webkit-reviews] review granted: [Bug 227690] ReadableStream's pipeTo() and pipeThrough() don't handle options in spec-perfect way : [Attachment 434779] Patch

[bugzilla-daemon at webkit.org]

Alexey Shvayka <shvaikalesh at gmail.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 227690: ReadableStream's pipeTo() and pipeThrough() don't handle options in
spec-perfect way
https://bugs.webkit.org/show_bug.cgi?id=227690

Attachment 434779: Patch

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




--- Comment #10 from Alexey Shvayka <shvaikalesh at gmail.com> ---
Comment on attachment 434779
  --> https://bugs.webkit.org/attachment.cgi?id=434779
Patch

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

r=me with 2 nits. Thank you for fixing this, Youenn!

> Source/WebCore/ChangeLog:9
> +	   Make one getter for signal.

Technically, `in` doesn't invoke a getter, only a Proxy trap.
Apart from not invoking getters on Object.prototype and tweaking their order,
this patch also fixed `options = null` case, added exception for primitive
`options`, and fixed exception handling in pipeTo().
It would be nice to mention that in ChangeLog.

> Source/WebCore/Modules/streams/ReadableStream.js:182
> +	       try {

It's nice to have this fixed.
A suggestion: what if we limit try { ... } scope only to rethrow userland
errors, while returning rejected Promises for non-object `options` and
incorrect `signal`? Like


    if (!@isObject(options))
	return @Promise. at reject(@makeTypeError("options must be an object"));

    try {
	preventAbort = !!options["preventAbort"];
	preventCancel = !!options["preventCancel"];
	preventClose = !!options["preventClose"];

	signal = options["signal"];
    } catch(e) {
	return @Promise. at reject(e);
    }

    if (signal !== @undefined && !(signal instanceof @AbortSignal))
	return @Promise. at reject(@makeTypeError("options.signal must be
AbortSignal"));


This way it's more clear that we are handling getter errors, and it's
consistent with the rest of the checks like if
(!@isWritableStream(destination)) etc.
`instanceof` can't throw for @AbortSignal (and it will be replaced with
@isAbortSignal after this patch is landed).