Bug 164014 – [Readable Streams API] Enable creation of ReadableByteStreamController

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

[Romain Bellessort]

Streams API defines that in addition to ReadableStreamDefaultController, ReadableByteStreamController may also be used. The first step toward this should be to enable the creation of a ReadableByteStreamController.

[Romain Bellessort]

Created attachment 292941 [details]
Patch

[WebKit Commit Bot]

Attachment 292941 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Source/WebCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebCore/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebKit/mac/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebKit2/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Source/WebKit2/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
Total errors found: 8 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 292941 [details]
Patch

Attachment 292941 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2379687

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 292950 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Romain Bellessort]

Created attachment 293005 [details]
Patch

[youenn fablet]

Comment on attachment 293005 [details]
Patch

Overall, looks to go in the right direction.

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

> Source/WTF/wtf/FeatureDefines.h:690
> +#if !defined(ENABLE_READABLE_STREAM_API_BYTESTREAM_PART)

Why not ENABLE_READABLE_BYTE_STREAM_API?

> Source/WebCore/DerivedSources.cpp:394
>  #if ENABLE(READABLE_STREAM_API)

Probably not needed since it should be defined in the cpp.

> Source/WebCore/DerivedSources.cpp:395
> +#if ENABLE(READABLE_STREAM_API_BYTESTREAM_PART)

Ditto.

> Source/WebCore/Modules/streams/ReadableByteStreamController.idl:31
> +    Conditional=READABLE_STREAM_API_BYTESTREAM_PART,

Should probably be conditional to READABLE_STREAM_API and READABLE_BYTE_STREAM_API in theory.

> Source/WebCore/Modules/streams/ReadableByteStreamController.js:26
> +// @conditional=ENABLE(READABLE_STREAM_API_BYTESTREAM_PART)

Ditto.

> Source/WebCore/Modules/streams/ReadableByteStreamController.js:57
> +    @throwTypeError("ReadableByteStreamController byobRequest() is not implemented");

Remove() since this is a getter.

> Source/WebCore/Modules/streams/ReadableByteStreamController.js:65
> +    @throwTypeError("ReadableByteStreamController desiredSize() is not implemented");

Ditto,

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:28
> +

Unneeded line

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:35
> +        @throwTypeError("ReadableByteStreamController needs a ReadableStream");

You probably want to use same formatting messages as other functions.
Messages should be something like:  Argument 1 ('stream') to XX must be an instance of ReadableStream.
JS built-ins do not use these messages and we should try to improve on this.
A start has been made by introducing things such as @makeThisTypeError.

But really, we are controlling the calling site of this function.
So we are probably sure stream is a stream.
We might want to write @assert(@isReadableStream(stream)) instead?
Also applicable to regular stream controller I guess.

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:50
> +    this.@strategy = @validateAndNormalizeQueuingStrategy(function() {return 1}, highWaterMark);

Missing ; before }

Would it not be better to align with the spec here and add a specific function for highWaterMark ?
Or do the processing here directly.

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:69
> +        //@readableByteStreamControllerCallPullIfNeeded(controller);

Not sure this is good to have commented code lines.
FIXME might be good enough.

> Source/WebCore/Modules/streams/ReadableStream.js:61
> +            this.@readableStreamController = new @ReadableByteStreamController(this, underlyingSource, strategy.highWaterMark);

Can we just make a test on self.@ReadableByteStreamController being not undefined?

> Tools/Scripts/webkitperl/FeatureList.pm:404
> +      define => "ENABLE_READABLE_STREAM_API_BYTESTREAM_PART", default => isGtk(), value => \$readableStreamAPIByteStreamPartSupport },

Why not mac as well?

> LayoutTests/platform/gtk/TestExpectations:2962
> +streams/readable-byte-stream-controller.html [ Pass ]

There should be a test for worker as well ideally.
You could do that by migrating all tests to LayoutTests/streams/readable-byte-stream-controller.js

It would also be nice to have tests printing the different error messages. These tests would be WebKit specific.
For those, it may be better to use js-test-pre/post.js

> LayoutTests/streams/readable-byte-stream-controller.html:5
> +// This is updated till https://github.com/whatwg/streams/commit/4ba861e6f60c248060811830e11271c84b439cc3

Probably unneeded

[Romain Bellessort]

Created attachment 293154 [details]
Patch

[Romain Bellessort]

(In reply to comment #6)
> Comment on attachment 293005 [details]
> Patch
> 
> Overall, looks to go in the right direction.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293005&action=review
> 
> > Source/WTF/wtf/FeatureDefines.h:690
> > +#if !defined(ENABLE_READABLE_STREAM_API_BYTESTREAM_PART)
> 
> Why not ENABLE_READABLE_BYTE_STREAM_API?

Better than my proposal indeed, I used this in the new patch.

> > Source/WebCore/DerivedSources.cpp:394
> >  #if ENABLE(READABLE_STREAM_API)
> 
> Probably not needed since it should be defined in the cpp.
> 
> > Source/WebCore/DerivedSources.cpp:395
> > +#if ENABLE(READABLE_STREAM_API_BYTESTREAM_PART)
> 
> Ditto.

I removed both.

> > Source/WebCore/Modules/streams/ReadableByteStreamController.idl:31
> > +    Conditional=READABLE_STREAM_API_BYTESTREAM_PART,
> 
> Should probably be conditional to READABLE_STREAM_API and
> READABLE_BYTE_STREAM_API in theory.
> 
> > Source/WebCore/Modules/streams/ReadableByteStreamController.js:26
> > +// @conditional=ENABLE(READABLE_STREAM_API_BYTESTREAM_PART)
> 
> Ditto.

I made this conditional in both cases.

> > Source/WebCore/Modules/streams/ReadableByteStreamController.js:57
> > +    @throwTypeError("ReadableByteStreamController byobRequest() is not implemented");
> 
> Remove() since this is a getter.
> 
> > Source/WebCore/Modules/streams/ReadableByteStreamController.js:65
> > +    @throwTypeError("ReadableByteStreamController desiredSize() is not implemented");
> 
> Ditto,

Done.

> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:28
> > +
> 
> Unneeded line

Removed.

> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:35
> > +        @throwTypeError("ReadableByteStreamController needs a ReadableStream");
> 
> You probably want to use same formatting messages as other functions.
> Messages should be something like:  Argument 1 ('stream') to XX must be an
> instance of ReadableStream.
> JS built-ins do not use these messages and we should try to improve on this.
> A start has been made by introducing things such as @makeThisTypeError.
> 
> But really, we are controlling the calling site of this function.
> So we are probably sure stream is a stream.
> We might want to write @assert(@isReadableStream(stream)) instead?
> Also applicable to regular stream controller I guess.

I agree, this is applicable to regular stream controller. Therefore, I suggest addressing this in a follow up patch.

> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:50
> > +    this.@strategy = @validateAndNormalizeQueuingStrategy(function() {return 1}, highWaterMark);
> 
> Missing ; before }

Fixed.

> Would it not be better to align with the spec here and add a specific
> function for highWaterMark ?
> Or do the processing here directly.

I have modified to align with spec. My intent was originally to reuse existing code as much as possible, but aligning is spec is clearer.

> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:69
> > +        //@readableByteStreamControllerCallPullIfNeeded(controller);
> 
> Not sure this is good to have commented code lines.
> FIXME might be good enough.

I removed commented code lines and kept only FIXMEs.

> > Source/WebCore/Modules/streams/ReadableStream.js:61
> > +            this.@readableStreamController = new @ReadableByteStreamController(this, underlyingSource, strategy.highWaterMark);
> 
> Can we just make a test on self.@ReadableByteStreamController being not
> undefined?

I tried various things but they didn’t work. Maybe we can keep this for the moment?

> > Tools/Scripts/webkitperl/FeatureList.pm:404
> > +      define => "ENABLE_READABLE_STREAM_API_BYTESTREAM_PART", default => isGtk(), value => \$readableStreamAPIByteStreamPartSupport },
> 
> Why not mac as well?

The new patch enables the feature for all ports. Initially, I thought that enabling tests only for GTK may be good as the implementation is very early.

> > LayoutTests/platform/gtk/TestExpectations:2962
> > +streams/readable-byte-stream-controller.html [ Pass ]
> 
> There should be a test for worker as well ideally.
> You could do that by migrating all tests to
> LayoutTests/streams/readable-byte-stream-controller.js

I added worker tests in the new patch. In the end, I am planning to add the tests to LayoutTests/imported/w3c/web-platform-tests/streams/readable-streams/ .

> It would also be nice to have tests printing the different error messages.
> These tests would be WebKit specific.
> For those, it may be better to use js-test-pre/post.js

As this is generally true for Readable Stream API, I think it could be done in a dedicated follow up patch. 

> > LayoutTests/streams/readable-byte-stream-controller.html:5
> > +// This is updated till https://github.com/whatwg/streams/commit/4ba861e6f60c248060811830e11271c84b439cc3
> 
> Probably unneeded

Removed.

[youenn fablet]

Comment on attachment 293154 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableStream.js:60
> +        try {

How about:
if (!@ReadableByteStreamController)
   @throwTypeError("ReadableByteStreamController is not supported");

> Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:28
> +#if ENABLE(READABLE_BYTE_STREAM_API)

Probably no needed, it should be in JSReadableByteStreamController.h already

> Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:37
> +#if ENABLE(READABLE_BYTE_STREAM_API)

Ditto

> Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:87
> +typedef JSBuiltinConstructor<JSReadableByteStreamController> JSBuiltinReadableByteStreamControllerPrivateConstructor;

We prefer "using" against "typedef" in terms of style.

> LayoutTests/TestExpectations:757
> +# streams/readable-byte-stream-controller.html [ Skip ]

Should we activate them?

> LayoutTests/streams/readable-byte-stream-controller.html:4
> +<script src='../imported/w3c/web-platform-tests/service-workers/service-worker/resources/test-helpers.sub.js'></script>

Hum, I don't really like that we start using wpt scripts outside of wpt directory.
In particular the .sub.js means that the file should be used through wpt server.

Since these tests are not ready yet for being upstreamed, this might be ok but please add a FIXME in these tests.

If you do no plan to upstream them to W3C, I would just do like done for fetch or add a helper routine somewhere to create test harness worker tests.

> LayoutTests/streams/readable-byte-stream-controller.js:1
> +'use strict';

Rethinking a bit these tests.
It might be better to try having tests that do something right and they will fail due to not implemented errors.
Tha way, these tests could be upstreamed to W3C WPT as well.

> LayoutTests/streams/readable-byte-stream-controller.js:7
> +

Remove one line here

> LayoutTests/streams/readable-byte-stream-controller.js:24
> +    assert_throws(new TypeError(), function() { controller.error(); });

As per comment above, I would just do controller.error() and then try to read so as to receive the error.
I would then rename the test also.

> LayoutTests/streams/readable-byte-stream-controller.js:27
> +

Ditto.

> LayoutTests/streams/readable-byte-stream-controller.js:37
> +    assert_throws(new TypeError(), function() { controller.close(); });

Ditto here

> LayoutTests/streams/readable-byte-stream-controller.js:49
> +    assert_throws(new TypeError(), function() { controller.enqueue(); });

Ditto here.

[Romain Bellessort]

Created attachment 293419 [details]
Patch

[Romain Bellessort]

(In reply to comment #9)
> Comment on attachment 293154 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293154&action=review
> 
> > Source/WebCore/Modules/streams/ReadableStream.js:60
> > +        try {
> 
> How about:
> if (!@ReadableByteStreamController)
>    @throwTypeError("ReadableByteStreamController is not supported");

I had tried this, but when the feature is disabled, instead of throwing the expected TypeError, it throws an error due to @ReadableByteStreamController private variable not being defined. I agree the try/catch approach may not be the cleanest, but it allows having a single place in the code impacted (which is indicated by a FIXME).

> > Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:28
> > +#if ENABLE(READABLE_BYTE_STREAM_API)
> 
> Probably no needed, it should be in JSReadableByteStreamController.h already
> 
> > Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:37
> > +#if ENABLE(READABLE_BYTE_STREAM_API)
> 
> Ditto

Removed.

> > Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:87
> > +typedef JSBuiltinConstructor<JSReadableByteStreamController> JSBuiltinReadableByteStreamControllerPrivateConstructor;
> 
> We prefer "using" against "typedef" in terms of style.
 
Updated with "using".

> > LayoutTests/TestExpectations:757
> > +# streams/readable-byte-stream-controller.html [ Skip ]
> 
> Should we activate them?

I activated them and removed said line.

> > LayoutTests/streams/readable-byte-stream-controller.html:4
> > +<script src='../imported/w3c/web-platform-tests/service-workers/service-worker/resources/test-helpers.sub.js'></script>
> 
> Hum, I don't really like that we start using wpt scripts outside of wpt
> directory.
> In particular the .sub.js means that the file should be used through wpt
> server.
> 
> Since these tests are not ready yet for being upstreamed, this might be ok
> but please add a FIXME in these tests.
> 
> If you do no plan to upstream them to W3C, I would just do like done for
> fetch or add a helper routine somewhere to create test harness worker tests.

I updated the tests to use fetch_tests_from_worker, which allows removing wpt script.

> > LayoutTests/streams/readable-byte-stream-controller.js:1
> > +'use strict';
> 
> Rethinking a bit these tests.
> It might be better to try having tests that do something right and they will
> fail due to not implemented errors.
> Tha way, these tests could be upstreamed to W3C WPT as well.

I rewrote the tests as you suggested.

> > LayoutTests/streams/readable-byte-stream-controller.js:7
> > +
> 
> Remove one line here

Done.

> > LayoutTests/streams/readable-byte-stream-controller.js:24
> > +    assert_throws(new TypeError(), function() { controller.error(); });
> 
> As per comment above, I would just do controller.error() and then try to
> read so as to receive the error.
> I would then rename the test also.
> 
> > LayoutTests/streams/readable-byte-stream-controller.js:27
> > +
> 
> Ditto.
> 
> > LayoutTests/streams/readable-byte-stream-controller.js:37
> > +    assert_throws(new TypeError(), function() { controller.close(); });
> 
> Ditto here
> 
> > LayoutTests/streams/readable-byte-stream-controller.js:49
> > +    assert_throws(new TypeError(), function() { controller.enqueue(); });
> 
> Ditto here.

[youenn fablet]

Comment on attachment 293419 [details]
Patch

r=me
The tests would be good to be uploaded to wpt

[WebKit Commit Bot]

Comment on attachment 293419 [details]
Patch

Rejecting attachment 293419 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 293419, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
 file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/streams/readable-byte-stream-controller-expected.txt
patching file LayoutTests/streams/readable-byte-stream-controller.html
patching file LayoutTests/streams/readable-byte-stream-controller.js
patching file ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Youenn Fablet']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/2448931

[Romain Bellessort]

Created attachment 293651 [details]
Patch

[Romain Bellessort]

Comment on attachment 293651 [details]
Patch

I rebased the patch, it should now be ok.

[WebKit Commit Bot]

Comment on attachment 293651 [details]
Patch

Clearing flags on attachment: 293651

Committed r208276: <http://trac.webkit.org/changeset/208276>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.