Re: [streams] Move stuff into controller to make RS and RBS closer (#418)

Review notes for readable-stream.js since I can't comment inline on such a big diff :). Overall looks quite solid; all my comments are fairly minor. I'm excited.

Per http://w3ctag.github.io/design-principles/#casing-rules we should do s/Byob/BYOB everywhere.

We should come up with a different name than ReadableStreamController/ReadableStreamReader since it is confusing; to me I would think both the BYOB and the normal one are "readable stream controllers/readers". Maybe ReadableStreamDefaultController and ReadableStreamDefaultReader? Also maybe rename ReadableByteStreamController to ReadableStreamBYOBController?

_controller should be _readableStreamController otherwise it's easy for other specs to accidentally collide with an internal slot named [[controller]].

It seems like parts of releaseLock are duplicated and can be moved into ReleaseReadableStreamReaderGeneric?

> ```js
>     this._cancel = CancelReadableStreamController;
>    this._pull = PullFromReadableStreamController;
> ```

Are you planning to move to the internal methods idea discussed in https://github.com/whatwg/streams/pull/418#issuecomment-165598765 ? I think in the reference implementation I would inline the method definitions in the class (`_cancel() { ... }`). In the spec they would be defined under the class as well.

I wonder if we should use methods for everything instead of just the polymorphic operations? Hmm let's stick with just the polymorphic ones for now. But maybe it will make things clearer eventually now that we have this more complex setup. I can do it in an editorial follow-up if we decide to go that route.

> ```js
>  constructor(controlledReadableStream, underlyingByteSource, size, highWaterMark) {
> ```

Size seems unused. I don't think the signatures need to match, so we can probably remove it.

> ```js
> if (Number.isNaN(highWaterMark)) {
> ```

maybe pull out a ValidateAndNormalizeHighWaterMark to share beween ValidateAndNormalizeQueuingStrategy and here.

> ReadableStream API exposed for controllers.

These should probably generally get asserts along the lines of IsReadableStreamReader(stream@[[reader]]) and IsReadableStreamBYOBReader(stream@[[reader]]) to make it clear which apply to which.

> ```js
> // A client of ReadableStreamController may use this function directly to bypass state check.
> ```

The state checks should probably be added as asserts in all cases possible.

> FillPullIntoDescriptorFromQueue

This reminded me that we should consider how we're going to divide these when speccing them, and try to arrange them correspondingly.

I am not sure I have all the categories but maybe something like:

- ReadableStream Abstract Operations
  - Utilities for Other Specifications (includes IsReadableStreamDisturbed but also AcquireReadableStreamReader, TeeReadableStream, ...)
  - Type-checking Helpers
  - Operations on `ReadableStream`s, Used by Controllers
    - will also include a note about Utilities from Other Specifications that can be used here
    - will include a note that ReleaseReadableStreamReaderGeneric can be used here (to avoid the wrapper ReleaseReadableStreamReaderGenericForController)
  - Operations on Readers (used by anyone in particular??)
  - Operations for `ReadableStreamController`s
  - Operations for `ReadableByteStreamController`s
    - this is where we put helpers like FillPullIntoDescriptorFromQueue

> TransferArrayBuffer

should move to helpers.js

---
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/418#issuecomment-171822826