Bug 146111 – [Streams API] Implement ReadableStream cancel (abstract part)

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

[youenn fablet]

We should implement https://streams.spec.whatwg.org/#cancel-readable-stream, at least the abstract part

[youenn fablet]

Created attachment 255114 [details]
Patch

[youenn fablet]

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.

[youenn fablet]

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.

[youenn fablet]

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

[youenn fablet]

Created attachment 255186 [details]
Fixing TypeError

[Darin Adler]

(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.

[Darin Adler]

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.

[youenn fablet]

Created attachment 255345 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 255345 [details]
Patch for landing

Clearing flags on attachment: 255345

Committed r185826: <http://trac.webkit.org/changeset/185826>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.