Bug 145210 – [Streams API] Implement ReadableStreamController enqueue

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Implement https://streams.spec.whatwg.org/#rs-controller-enqueue, including resolution of read promise.

[youenn fablet]

Created attachment 253527 [details]
Patch

[youenn fablet]

Created attachment 254162 [details]
Patch

[Darin Adler]

Comment on attachment 254162 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableStream.cpp:172
> +    m_readRequests.first().successCallback(value);
> +    m_readRequests.remove(0);

The logical operation to use here is takeFirst. Vector doesn’t have a convenience for takeFirst because using takeFirst is a clue that you should not be using a Vector, but rather a Deque; Vector has fast takeLast, but slow takeFirst. Actually calling "remove(0)" is an anti-pattern.

> Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:59
> +    if (stream.isErrored())
> +        return exec->vm().throwException(exec, stream.error());
> +    if (stream.isCloseRequested())
> +        return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Calling enqueue on a stream which is closing")));

Do we have test coverage for both of these? I couldn’t find it.

It is not good that we need to do a custom binding here. You aren’t supposed to need custom bindings just to raise an exception. It’s not good that the rules about when it’s OK to call this function are in the binding, rather than in the DOM class. We want the binding code to be solely about binding, not about the semantics of the class, if at all possible.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:162
> +    JSValue chunk = m_chunkQueue.first().get();
> +    m_chunkQueue.remove(0);
> +    return chunk;

Same comment about takeFirst as above. This should be a Deque, not a Vector.

[youenn fablet]

Comment on attachment 254162 [details]
Patch

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

>> Source/WebCore/Modules/streams/ReadableStream.cpp:172
>> +    m_readRequests.remove(0);
> 
> The logical operation to use here is takeFirst. Vector doesn’t have a convenience for takeFirst because using takeFirst is a clue that you should not be using a Vector, but rather a Deque; Vector has fast takeLast, but slow takeFirst. Actually calling "remove(0)" is an anti-pattern.

Thanks for pointing me to Deque, I will apply to value queue when landing this patch.

For the callback queue, I may need to add Deque::append similarly to Vector::append.
If so, I will do that as a follow-up patch.

Any reason to not use STL dedicated containers btw?

>> Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:59
>> +        return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Calling enqueue on a stream which is closing")));
> 
> Do we have test coverage for both of these? I couldn’t find it.
> 
> It is not good that we need to do a custom binding here. You aren’t supposed to need custom bindings just to raise an exception. It’s not good that the rules about when it’s OK to call this function are in the binding, rather than in the DOM class. We want the binding code to be solely about binding, not about the semantics of the class, if at all possible.

There is code coverage for both in streams/reference-implementation/readable-stream.html
Tests are named as:
PASS ReadableStream: enqueue should throw when the stream is closed 
PASS ReadableStream: enqueue should throw the stored error when the stream is errored 

As of where to put the exception throwing, it is not straightforward to do that within the DOM class.
Binding code is expected to pass exceptions/errors as ExceptionCode.
But we may want to throw a generic JSValue for instance if stream is errored.

Also ReadableStreamContoller is a special DOM class.
It is only expected to be used with ReadableJSStream and we could have emulated it as a pure JS class.

Note also that the separation of code between ReadableStreamController binding code and ReadableJSStream tries to follow the organisation of the spec.

>> Source/WebCore/bindings/js/ReadableJSStream.cpp:162
>> +    return chunk;
> 
> Same comment about takeFirst as above. This should be a Deque, not a Vector.

OK

[youenn fablet]

Created attachment 254261 [details]
Patch for landing, including using Deque for stack of JSValue

[WebKit Commit Bot]

Comment on attachment 254261 [details]
Patch for landing, including using Deque for stack of JSValue

Clearing flags on attachment: 254261

Committed r185197: <http://trac.webkit.org/changeset/185197>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #4)
> Any reason to not use STL dedicated containers btw?

It’s hard to answer that question; it's a really broad one because there are a lot of containers.

One issue is that std::deque would use the system malloc, not the faster bmalloc used by WTF::Deque. Another is that std::vector is missing many of the features of WTF::Vector. Both class templates might have quite different memory characteristics than their WTF counterparts in terms of overhead and things like growth/shrinking strategies.

Something we’d definitely have to discuss with others in the project. Not an easy choice.