Bug 146111

Created attachment 255114 [details] Patch

Comment on attachment 255114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255114&action=review > Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:-48 > - return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Calling close on a stream which is not readable"))); This change is related to the note at https://streams.spec.whatwg.org/#close-readable-stream > Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:52 > + // FIXME: We should be able to remove this custom binding, once we can pass a JSValue or a ScriptValue. I tried removing the custom binding now that we have a functional binding generator. But we end up with using a deprecated ScriptValue. I do not know the reason of this deprecation. Any idea? Currently, we are passing reason as a JSValue. This is ok for ReadableJSStream as it can get access to ExecState and the likes in other ways. Having a ScriptValue might be handy for native sources, should they feel the need to use the reason. But I am not sure native sources will really care about the reason anyway. The binding generator could be fixed either by undeprecating ScriptValue or directly using JSValue.

Comment on attachment 255114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255114&action=review > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:43 > + promise.resolve(nullptr); This last-minute change is wrong, sorry about that... I will move that check temporarily in the custom binding. Then, once we will remove the custom binding, we can use RaisesException to move back this check in ReadableStream.

Comment on attachment 255114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255114&action=review >> Source/WebCore/Modules/streams/ReadableStreamReader.cpp:43 >> + promise.resolve(nullptr); > > This last-minute change is wrong, sorry about that... > I will move that check temporarily in the custom binding. > Then, once we will remove the custom binding, we can use RaisesException to move back this check in ReadableStream. Forget about this comment, it applies partially to ReadableStream::cancel

Created attachment 255186 [details] Fixing TypeError

(In reply to comment #2) > But we end up with using a deprecated ScriptValue. > I do not know the reason of this deprecation. Any idea? ScriptValue was an abstraction to support using multiple JavaScript engines with WebKit. It adds unnecessary overhead; we can use JavaScriptCore concepts directly now.

Comment on attachment 255186 [details] Fixing TypeError View in context: https://bugs.webkit.org/attachment.cgi?id=255186&action=review > Source/WebCore/Modules/streams/ReadableStream.h:51 > +typedef int ExceptionCode; Should be in a separate paragraph. Blank line before it. > Source/WebCore/Modules/streams/ReadableStream.h:123 > + std::unique_ptr<CancelPromise> m_cancelPromise; We should consider Optional here instead of std::unique_ptr. There’s no polymorphism to force us to put this on the heap, so it’s a tradeoff between extra storage all the time when there is no cancel promise vs. slowness of heap allocation and even more storage because of heap block overhead when there is a cancel promise. > Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:54 > + JSPromiseDeferred* promiseDeferred = JSPromiseDeferred::create(exec, globalObject()); I think a reference would be better than a pointer here. > Source/WebCore/bindings/js/JSReadableStreamReaderCustom.cpp:92 > + JSPromiseDeferred* promiseDeferred = JSPromiseDeferred::create(exec, globalObject()); I think a reference would be better than a pointer here.

Created attachment 255345 [details] Patch for landing

Comment on attachment 255345 [details] Patch for landing Clearing flags on attachment: 255345 Committed r185826: <http://trac.webkit.org/changeset/185826>

All reviewed patches have been landed. Closing bug.