Bug 146111 - [Streams API] Implement ReadableStream cancel (abstract part)
Summary: [Streams API] Implement ReadableStream cancel (abstract part)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 138967
  Show dependency treegraph
 
Reported: 2015-06-18 07:01 PDT by youenn fablet
Modified: 2015-06-22 06:59 PDT (History)
4 users (show)

See Also:


Attachments
Patch (27.93 KB, patch)
2015-06-18 07:30 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing TypeError (28.49 KB, patch)
2015-06-19 04:17 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (28.64 KB, patch)
2015-06-22 06:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2015-06-18 07:01:02 PDT
We should implement https://streams.spec.whatwg.org/#cancel-readable-stream, at least the abstract part
Comment 1 youenn fablet 2015-06-18 07:30:15 PDT
Created attachment 255114 [details]
Patch
Comment 2 youenn fablet 2015-06-18 07:42:12 PDT
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 3 youenn fablet 2015-06-18 08:20:10 PDT
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 4 youenn fablet 2015-06-18 08:45:53 PDT
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
Comment 5 youenn fablet 2015-06-19 04:17:13 PDT
Created attachment 255186 [details]
Fixing TypeError
Comment 6 Darin Adler 2015-06-19 10:53:06 PDT
(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 7 Darin Adler 2015-06-19 10:56:43 PDT
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.
Comment 8 youenn fablet 2015-06-22 06:03:21 PDT
Created attachment 255345 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2015-06-22 06:59:55 PDT
Comment on attachment 255345 [details]
Patch for landing

Clearing flags on attachment: 255345

Committed r185826: <http://trac.webkit.org/changeset/185826>
Comment 10 WebKit Commit Bot 2015-06-22 06:59:59 PDT
All reviewed patches have been landed.  Closing bug.