Re: [whatwg/streams] Support teeing readable byte streams (#1114)

@domenic approved this pull request.

LGTM with nits. Exciting...

> @@ -2055,6 +2044,29 @@ The following abstract operations operate on {{ReadableStream}} instances at a h
  |startAlgorithm| throws.
 </div>
 
+<div algorithm>
+ <dfn abstract-op lt="CreateReadableByteStream"
+ id="create-readable-byte-stream">CreateReadableByteStream(|startAlgorithm|, |pullAlgorithm|,
+ |cancelAlgorithm|[, |highWaterMark|, [, |autoAllocateChunkSize|]])</dfn> performs the following

Since nobody ever passes the other args, maybe omit them?

> @@ -2055,6 +2044,29 @@ The following abstract operations operate on {{ReadableStream}} instances at a h
  |startAlgorithm| throws.
 </div>
 
+<div algorithm>
+ <dfn abstract-op lt="CreateReadableByteStream"
+ id="create-readable-byte-stream">CreateReadableByteStream(|startAlgorithm|, |pullAlgorithm|,

I think we landed on no manual ID curation for new abstract ops?

> @@ -2218,6 +2230,19 @@ create them does not matter.
 
  It performs the following steps:
 
+ 1. Assert: |stream| [=implements=] {{ReadableStream}}.
+ 1. Assert: |cloneForBranch2| is a boolean.
+ 1. If |stream|.[=ReadableStream/[[controller]]=] [=implements=] {{ReadableByteStreamController}},
+  1. Return ? [$ReadableByteStreamTee$](|stream|).

Nit: although we're not consistent, recently I've preferred folding single-line blocks into their parents (i.e. "If X, y" instead of two separate list items). Unless that would break symmetry between If/Otherwise, I guess.

> + 1. Assert: |stream| [=implements=] {{ReadableStream}}.
+ 1. Assert: |stream|.[=ReadableStream/[[controller]]=] [=implements=]
+    {{ReadableByteStreamController}}.
+ 1. Let |reader| be ? [$AcquireReadableStreamDefaultReader$](|stream|).
+ 1. Let |reading| be false.
+ 1. Let |canceled1| be false.
+ 1. Let |canceled2| be false.
+ 1. Let |reason1| be undefined.
+ 1. Let |reason2| be undefined.
+ 1. Let |branch1| be undefined.
+ 1. Let |branch2| be undefined.
+ 1. Let |cancelPromise| be [=a new promise=].
+ 1. Let |forwardReaderError| be the following steps, taking a |thisReader| argument:
+  1. [=Upon rejection=] of |thisReader|.[=ReadableStreamGenericReader/[[closedPromise]]=] with reason
+     |r|,
+   1. If |reader| is not |thisReader|, return.

Makes more sense to me flipped, as if thisReader is not reader?

> +  1. [=Upon rejection=] of |thisReader|.[=ReadableStreamGenericReader/[[closedPromise]]=] with reason
+     |r|,
+   1. If |reader| is not |thisReader|, return.
+   1. Perform ! [$ReadableByteStreamControllerError$](|branch1|.[=ReadableStream/[[controller]]=],
+      |r|).
+   1. Perform ! [$ReadableByteStreamControllerError$](|branch2|.[=ReadableStream/[[controller]]=],
+      |r|).
+   1. If |canceled1| is false or |canceled2| is false, [=resolve=] |cancelPromise| with undefined.
+ 1. Let |pullWithDefaultReader| be the following steps:
+  1. If |reader| [=implements=] {{ReadableStreamBYOBReader}},
+   1. Assert: |reader|.[=ReadableStreamBYOBReader/[[readIntoRequests]]=] is [=list/is empty|empty=].
+   1. Perform ! [$ReadableStreamReaderGenericRelease$](|reader|).
+   1. Set |reader| to ! [$AcquireReadableStreamDefaultReader$](|stream|).
+   1. Perform |forwardReaderError|, given |reader|.
+  1. Let |readRequest| be a [=read request=] with the following [=struct/items=]:
+   : [=read request/chunk steps=], given |chunk|

We should use either _chunk_ or _value_ as variable names uniformly between the two algorithms. I'm OK with either, although I suspect _value_ is more prevalent in other parts of the spec?

> +   1. Set |reader| to ! [$AcquireReadableStreamBYOBReader$](|stream|).
+   1. Perform |forwardReaderError|, given |reader|.
+  1. Let |byobBranch| be |branch2| if |forBranch2| is true, and |branch1| otherwise.
+  1. Let |otherBranch| be |branch2| if |forBranch2| is false, and |branch1| otherwise.
+  1. Let |readIntoRequest| be a [=read-into request=] with the following [=struct/items=]:
+   : [=read-into request/chunk steps=], given |chunk|
+   ::
+    1. [=Queue a microtask=] to perform the following steps:
+     1. Set |reading| to false.
+     1. Let |byobCanceled| be |canceled2| if |forBranch2| is true, and |canceled1| otherwise.
+     1. Let |otherCanceled| be |canceled2| if |forBranch2| is false, and |canceled1| otherwise.
+     1. If |otherCanceled| is false,
+      1. Let |clonedChunk| be ? [$CloneAsUint8Array$](|chunk|).
+      1. Perform ! [$ReadableByteStreamControllerEnqueue$](|otherBranch|.[=ReadableStream/[[controller]]=],
+         |clonedChunk|).
+     1. If |byobCanceled| is false,

Nit: flip the order here, so byob is always before "other"?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/1114#pullrequestreview-676277696