Bug 141045 - [Streams API] Implement a barebone ReadableStream interface
Summary: [Streams API] Implement a barebone ReadableStream interface
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 141162
  Show dependency treegraph
 
Reported: 2015-01-29 12:56 PST by youenn fablet
Modified: 2015-02-23 07:04 PST (History)
7 users (show)

See Also:


Attachments
Patch (107.35 KB, patch)
2015-01-29 13:12 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (107.35 KB, patch)
2015-01-29 13:52 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (107.37 KB, patch)
2015-01-29 14:30 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing and minor style improvements (106.99 KB, patch)
2015-02-02 08:07 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (109.21 KB, patch)
2015-02-03 08:04 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (110.90 KB, patch)
2015-02-05 01:27 PST, 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-01-29 12:56:03 PST
Implement the ReadableStream IDL, the functionality behind being added progressively in future patches.
Comment 1 youenn fablet 2015-01-29 13:12:58 PST
Created attachment 245644 [details]
Patch
Comment 2 youenn fablet 2015-01-29 13:52:41 PST
Created attachment 245645 [details]
Patch
Comment 3 youenn fablet 2015-01-29 14:30:24 PST
Created attachment 245649 [details]
Patch
Comment 4 youenn fablet 2015-02-02 08:07:20 PST
Created attachment 245875 [details]
Rebasing and minor style improvements
Comment 5 Benjamin Poulain 2015-02-02 21:08:34 PST
Comment on attachment 245875 [details]
Rebasing and minor style improvements

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

Sorry, I only had time for a super quick review.

It is great you split the patch is smaller chunk, it is much easier to verify.

My main concerns so far are:
-It is weird JSReadableStream requires so much custom bindings. Custom Bindings are supposed to be an exception for performance related stuff. It may be better to extend the binding generator.
-Unless I am missing JSReadableStreamSource will be for, the name is incorrect. The JSPrefix is generally for objects on the JS heap, which is not the case here.

> Source/WebCore/Modules/streams/ReadableStream.cpp:84
> +    // FIXME: To be implemented

You can use notImplemented() to get logging of missing stuff.

> Source/WebCore/Modules/streams/ReadableStream.cpp:89
> +    // FIXME: To be implemented

diitto

> Source/WebCore/Modules/streams/ReadableStream.idl:36
> +    readonly  attribute ReadableStreamStateType state;

Two spaces between readonly and attribute.

Shouldn't this be [GetterRaisesException]? The spec seems to say this can throw if the readable stream has no source: https://streams.spec.whatwg.org/#rs-state

> Source/WebCore/Modules/streams/ReadableStream.idl:44
> +    [GetterRaisesException, CustomGetter] readonly attribute Promise closed;
> +    [GetterRaisesException, CustomGetter] readonly attribute Promise ready;

On the other hand I don't get why those two can raise. Shouldn't they just return a rejected promise?

> Source/WebCore/Modules/streams/ReadableStreamSource.h:49
> +        UNUSED_PARAM(queueSize);

We just omit the param in C++. 

UNUSED_PARAM is mostly for Obj-C.

> Source/WebCore/Modules/streams/ReadableStreamSource.h:56
> +        UNUSED_PARAM(reason);

ditto.

> ChangeLog:9
> +        * Source/cmake/WebKitFeatures.cmake:
> +        * Source/cmakeconfig.h.cmake: Made streams API compilation on by default.

It would be useful to Object.getOwnPropertyDescriptor() the whole interface. It's good to check we expose the attributes we expect.
Comment 6 youenn fablet 2015-02-03 05:45:01 PST
(In reply to comment #5)
> Comment on attachment 245875 [details]
> Rebasing and minor style improvements
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245875&action=review
> 
> Sorry, I only had time for a super quick review.

Thanks for taking that time, this is much appreciated!

> 
> It is great you split the patch is smaller chunk, it is much easier to
> verify.
> 
> My main concerns so far are:
> -It is weird JSReadableStream requires so much custom bindings. Custom
> Bindings are supposed to be an exception for performance related stuff. It
> may be better to extend the binding generator.

Currently use of promise requires going through custom bindings, which ReadableStream is all about, except for state and read().
The current patch tries as much as possible to have a JS-free ReadableStream definition.

It would be great to ease Promise usage, but it sounds a bit premature to me right now.
Looking at Crypto promise usage, it seems not straightforward to pick the right model that would suit most Promise-based APIs (error handling, resolve type at IDL level...).
I would be interested in looking further if/when there are other promise-based APIs on the go.

I guess the crux of the issue is that streams are really in between JS and WebCore.

> -Unless I am missing JSReadableStreamSource will be for, the name is
> incorrect. The JSPrefix is generally for objects on the JS heap, which is
> not the case here.

OK, let's rename it to ReadabbleStreamJSSource then.

> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:84
> > +    // FIXME: To be implemented
> 
> You can use notImplemented() to get logging of missing stuff.

OK
> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:89
> > +    // FIXME: To be implemented
> 
> diitto
> 
> > Source/WebCore/Modules/streams/ReadableStream.idl:36
> > +    readonly  attribute ReadableStreamStateType state;
> 
> Two spaces between readonly and attribute.
> 
> Shouldn't this be [GetterRaisesException]? The spec seems to say this can
> throw if the readable stream has no source:
> https://streams.spec.whatwg.org/#rs-state

A ReadableStream always has a source with the current design.
We can fix that later if there is a use case for a ReadableStream without a source.

> > Source/WebCore/Modules/streams/ReadableStream.idl:44
> > +    [GetterRaisesException, CustomGetter] readonly attribute Promise closed;
> > +    [GetterRaisesException, CustomGetter] readonly attribute Promise ready;
> 
> On the other hand I don't get why those two can raise. Shouldn't they just
> return a rejected promise?

Historical reasons...
I will fix that.

> 
> > Source/WebCore/Modules/streams/ReadableStreamSource.h:49
> > +        UNUSED_PARAM(queueSize);
> 
> We just omit the param in C++. 

OK

> 
> UNUSED_PARAM is mostly for Obj-C.
> 
> > Source/WebCore/Modules/streams/ReadableStreamSource.h:56
> > +        UNUSED_PARAM(reason);
> 
> ditto.
> 
> > ChangeLog:9
> > +        * Source/cmake/WebKitFeatures.cmake:
> > +        * Source/cmakeconfig.h.cmake: Made streams API compilation on by default.
> 
> It would be useful to Object.getOwnPropertyDescriptor() the whole interface.
> It's good to check we expose the attributes we expect.

I am not sure to get it.
LayoutTests/js/dom/global-constructors-attributes.html is capturing ReadableStream properties.
LayoutTests/streams/readablestream-constructor.html is checking each expected property and its type or value.
I will beef up a little bit this test to ensure that no unexpected attribute is exposed.
Comment 7 youenn fablet 2015-02-03 08:04:59 PST
Created attachment 245939 [details]
Patch
Comment 8 Benjamin Poulain 2015-02-03 15:34:09 PST
Adding the bindings generator expert.
Comment 9 Benjamin Poulain 2015-02-03 21:13:06 PST
> > > ChangeLog:9
> > > +        * Source/cmake/WebKitFeatures.cmake:
> > > +        * Source/cmakeconfig.h.cmake: Made streams API compilation on by default.
> > 
> > It would be useful to Object.getOwnPropertyDescriptor() the whole interface.
> > It's good to check we expose the attributes we expect.
> 
> I am not sure to get it.
> LayoutTests/js/dom/global-constructors-attributes.html is capturing
> ReadableStream properties.
> LayoutTests/streams/readablestream-constructor.html is checking each
> expected property and its type or value.
> I will beef up a little bit this test to ensure that no unexpected attribute
> is exposed.

Maybe I am the one missing the test.

You are adding 3 enumerable properties and a few calls, but I don't see their properties dumped anywhere (see LayoutTests/fast/dom/SelectorAPI/closest-definition.html for a recent example of missing test coverage).
Comment 10 Benjamin Poulain 2015-02-03 21:23:28 PST
Comment on attachment 245939 [details]
Patch

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

This patch is great. Let's move forward.

Please have a look at the extra tests for your Interface before landing. The more coverage we have the better.

> Source/WebCore/Modules/streams/ReadableStream.cpp:33
> +#include "ReadableStream.h"

Since ReadableStream.h includes the ENABLE(STREAMS_API) guards, you can use the regular style for includes:

#include "config.h"
#include "ReadableStream.h"

....

> Source/WebCore/Modules/streams/ReadableStream.cpp:56
> +    readableStreamCounter.increment();

You may want to add a dump() function with #ifndef NDEBUG to show the counter.

I have found such little helper to be useful for me when debugging.

> Source/WebCore/Modules/streams/ReadableStream.idl:30
> +enum ReadableStreamStateType { "readable",  "waiting", "closed", "errored" };

Two spaces between "readable" and "waiting"
Comment 11 youenn fablet 2015-02-05 01:27:41 PST
Created attachment 246094 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2015-02-05 02:19:10 PST
Comment on attachment 246094 [details]
Patch for landing

Clearing flags on attachment: 246094

Committed r179687: <http://trac.webkit.org/changeset/179687>
Comment 13 WebKit Commit Bot 2015-02-05 02:19:16 PST
All reviewed patches have been landed.  Closing bug.