Implement ReadableStreamController as defined in https://streams.spec.whatwg.org/#rs-controller-class
Created attachment 250603 [details] Patch
Created attachment 250605 [details] Trying to fix mac build
Comment on attachment 250605 [details] Trying to fix mac build View in context: https://bugs.webkit.org/attachment.cgi?id=250605&action=review > Source/WebCore/ChangeLog:1 > +2015-04-12 Youenn Fablet <youenn.fablet@crf.canon.fr> Do not forget me ;) > Source/WebCore/Modules/streams/ReadableStreamController.h:44 > +/* This class is only used for JS source readable streams to allow enqueuing, closing or erroring a readable stream. > + Its definition is at https://streams.spec.whatwg.org/#rs-controller-class. > + Note that its constructor is taking a ReadableJSStream as it should only be used for JS sources.*/ // Comments > LayoutTests/ChangeLog:1 > +2015-04-12 Youenn Fablet <youenn.fablet@crf.canon.fr> Me. > LayoutTests/streams/reference-implementation/readable-stream.html:78 > - > - assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'constructor', 'enqueue', 'error' ]); > + // FIXME: decide whether exposing the constructor. > + assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'enqueue', 'error' ]); I think we should do what the reference implementation does, which is exposing the constructor. Therefore this wouldn't be needed. > LayoutTests/streams/reference-implementation/readable-stream.html:94 > - assert_array_equals(Object.getOwnPropertyNames(enqueue).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]); > - assert_array_equals(Object.getOwnPropertyNames(close).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]); > - assert_array_equals(Object.getOwnPropertyNames(error).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]); > + assert_array_equals(Object.getOwnPropertyNames(enqueue).sort(), [ 'length', 'name' ]); > + assert_array_equals(Object.getOwnPropertyNames(close).sort(), [ 'length', 'name' ]); > + assert_array_equals(Object.getOwnPropertyNames(error).sort(), [ 'length', 'name' ]); > > - assert_equals(enqueue.name, ''); > - assert_equals(close.name, ''); > - assert_equals(error.name, ''); > + assert_equals(enqueue.name, 'enqueue'); > + assert_equals(close.name, 'close'); > + assert_equals(error.name, 'error'); This test has changed. It needs to be rebased. Also, I don't think we should be removing the things that are part of spec tests, we should comply. > LayoutTests/streams/reference-implementation/readable-stream.html:115 > - assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'constructor', 'enqueue', 'error' ]); > + assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'enqueue', 'error' ]); > controller.test = ""; > - assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'constructor', 'enqueue', 'error' ]); > + assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'enqueue', 'error' ]); Ditto.
(In reply to comment #3) > Comment on attachment 250605 [details] > Trying to fix mac build > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250605&action=review > > > Source/WebCore/ChangeLog:1 > > +2015-04-12 Youenn Fablet <youenn.fablet@crf.canon.fr> > > Do not forget me ;) Woups, sure. > > LayoutTests/streams/reference-implementation/readable-stream.html:78 > > - > > - assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'constructor', 'enqueue', 'error' ]); > > + // FIXME: decide whether exposing the constructor. > > + assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'enqueue', 'error' ]); > > I think we should do what the reference implementation does, which is > exposing the constructor. Therefore this wouldn't be needed. The binding generator is not supporting NoInterfaceObject and CustomConstructor, hence the issue. The constructor is always throwing an exception, hence not all that useful. This could be fixed in JSReadablestreamCustom.cpp later on. > > LayoutTests/streams/reference-implementation/readable-stream.html:94 > > - assert_array_equals(Object.getOwnPropertyNames(enqueue).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]); > > - assert_array_equals(Object.getOwnPropertyNames(close).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]); > > - assert_array_equals(Object.getOwnPropertyNames(error).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]); > > + assert_array_equals(Object.getOwnPropertyNames(enqueue).sort(), [ 'length', 'name' ]); > > + assert_array_equals(Object.getOwnPropertyNames(close).sort(), [ 'length', 'name' ]); > > + assert_array_equals(Object.getOwnPropertyNames(error).sort(), [ 'length', 'name' ]); > > > > - assert_equals(enqueue.name, ''); > > - assert_equals(close.name, ''); > > - assert_equals(error.name, ''); > > + assert_equals(enqueue.name, 'enqueue'); > > + assert_equals(close.name, 'close'); > > + assert_equals(error.name, 'error'); > > This test has changed. It needs to be rebased. Also, I don't think we should > be removing the things that are part of spec tests, we should comply. With testharness tests, the test stops after the first failed assertion. In that case, we are exiting the test without having tested any API really. By updating the test, we still test all implemented properties. I think we can update the tests once implementing the API. When finalizing, we can resync all tests.
Created attachment 250705 [details] Rebased patch
(In reply to comment #4) > > > LayoutTests/streams/reference-implementation/readable-stream.html:94 > > > - assert_array_equals(Object.getOwnPropertyNames(enqueue).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]); > > > - assert_array_equals(Object.getOwnPropertyNames(close).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]); > > > - assert_array_equals(Object.getOwnPropertyNames(error).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]); > > > + assert_array_equals(Object.getOwnPropertyNames(enqueue).sort(), [ 'length', 'name' ]); > > > + assert_array_equals(Object.getOwnPropertyNames(close).sort(), [ 'length', 'name' ]); > > > + assert_array_equals(Object.getOwnPropertyNames(error).sort(), [ 'length', 'name' ]); > > > > > > - assert_equals(enqueue.name, ''); > > > - assert_equals(close.name, ''); > > > - assert_equals(error.name, ''); > > > + assert_equals(enqueue.name, 'enqueue'); > > > + assert_equals(close.name, 'close'); > > > + assert_equals(error.name, 'error'); > > > > This test has changed. It needs to be rebased. Also, I don't think we should > > be removing the things that are part of spec tests, we should comply. > > With testharness tests, the test stops after the first failed assertion. > In that case, we are exiting the test without having tested any API really. > By updating the test, we still test all implemented properties. > > I think we can update the tests once implementing the API. > When finalizing, we can resync all tests. Commenting with Youenn on IRC we have two options here in order to not lose track of what we have now and what is the spec goal: 1. We could have in a comment which is the real goal of the test and keep running what we have now. 2. Keep the spec test failing and have a custom test that would be removed once the everything is implemented and the spec test passes. I was thinking that 1 would be better but maybe 2 is better if we refer to the goal spec test in the description of the custom one.
Comment on attachment 250705 [details] Rebased patch Attachment 250705 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4539333881102336 New failing tests: streams/readable-stream.html
Created attachment 250707 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 250705 [details] Rebased patch Attachment 250705 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5976024936349696 New failing tests: streams/readable-stream.html
Created attachment 250708 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
(In reply to comment #7) > Comment on attachment 250705 [details] > Rebased patch > > Attachment 250705 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.appspot.com/results/4539333881102336 > > New failing tests: > streams/readable-stream.html In this case, as tests are not in the spec yet, so I think we can update our custom test.
Comment on attachment 250705 [details] Rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=250705&action=review Was this supposed to be for review? I think this is going in the right direction. I don't think this covers the cases I mentioned in the other bug. > Source/WebCore/Modules/streams/ReadableStreamController.h:60 > + ReadableJSStream* m_stream; Please explain the lifetime of the objects in the Changelog. > Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:46 > + if (impl().stream().internalState() != ReadableStream::State::Readable) I would store impl().stream() in a temporary variable. > LayoutTests/streams/reference-implementation/brand-checks-expected.txt:9 > +FAIL ReadableStreamController enforces a brand check on its argument assert_throws: Constructing a ReadableStreamController should throw function "function () { new ReadableStreamController(fakeReadableSt..." did not throw Looks like we have a regression here.
(In reply to comment #12) > Comment on attachment 250705 [details] > Rebased patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250705&action=review > > Was this supposed to be for review? > > I think this is going in the right direction. I don't think this covers the > cases I mentioned in the other bug. > > > Source/WebCore/Modules/streams/ReadableStreamController.h:60 > > + ReadableJSStream* m_stream; > > Please explain the lifetime of the objects in the Changelog. Sounds good. > > > Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:46 > > + if (impl().stream().internalState() != ReadableStream::State::Readable) > > I would store impl().stream() in a temporary variable. OK > > LayoutTests/streams/reference-implementation/brand-checks-expected.txt:9 > > +FAIL ReadableStreamController enforces a brand check on its argument assert_throws: Constructing a ReadableStreamController should throw function "function () { new ReadableStreamController(fakeReadableSt..." did not throw > > Looks like we have a regression here. As long as the controller constructor is not implemented, all corresponding tests are not really relevant. I will upload a new patch without controller constructor. I do not really see the point of adding support for it but I will upload another patch dedicated to the constructor so that we can continue discussing that specific point.
Created attachment 250784 [details] Rebasing webkit specifc test
> I do not really see the point of adding support for it but I will upload > another patch dedicated to the constructor so that we can continue > discussing that specific point. Patch is available at https://bugs.webkit.org/show_bug.cgi?id=143752, on top of this latest patch.
Comment on attachment 250784 [details] Rebasing webkit specifc test Patch is rebased according current trunk. Compared to previous patch, it does not implement close() functionality. Plan is to add support for it in 143363 after this one is shipped, which is probably more logical that way.
Comment on attachment 250784 [details] Rebasing webkit specifc test View in context: https://bugs.webkit.org/attachment.cgi?id=250784&action=review > Source/WebCore/ChangeLog:12 > + A controller is created at the time a ReadableJSStream is started and it is owned by it. > + The controller will be destroyed when the ReadableJSStream is also destroyed. Sorry I did not have the time to review any stream patch today. :( Quick question: The ReadableStreamController has a wrapper that keep a reference to it. Can't the wrapper outlive the ReadableJSStream?
Created attachment 250907 [details] Fixing wrapper issue
(In reply to comment #17) > Comment on attachment 250784 [details] > Rebasing webkit specifc test > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250784&action=review > > > Source/WebCore/ChangeLog:12 > > + A controller is created at the time a ReadableJSStream is started and it is owned by it. > > + The controller will be destroyed when the ReadableJSStream is also destroyed. > > Sorry I did not have the time to review any stream patch today. :( > > Quick question: The ReadableStreamController has a wrapper that keep a > reference to it. Can't the wrapper outlive the ReadableJSStream? Good spot! Fixed the issue and added a related async test case. I also filed https://bugs.webkit.org/show_bug.cgi?id=143818 as the implementation of ReadableStreamJSSource::startAsync was defeating garbage collection purpose.
Comment on attachment 250907 [details] Fixing wrapper issue View in context: https://bugs.webkit.org/attachment.cgi?id=250907&action=review > Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:52 > + notImplemented(); Will be implemented within rebased patch of https://bugs.webkit.org/show_bug.cgi?id=143363 > LayoutTests/streams/reference-implementation/brand-checks-expected.txt:-10 > -PASS ReadableStreamController can't be given a fully-constructed ReadableStream These regressions are fixed in https://bugs.webkit.org/show_bug.cgi?id=143752
Comment on attachment 250907 [details] Fixing wrapper issue View in context: https://bugs.webkit.org/attachment.cgi?id=250907&action=review > Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:48 > + return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Controller has no reaadablestream"))); Typo reaadablestream > Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:50 > + if (stream->internalState() != ReadableStream::State::Readable) Trailing space at the end of the line > Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:60 > + return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Controller has no reaadablestream"))); Typo > Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:69 > + return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Controller has no reaadablestream"))); Typo
Created attachment 250948 [details] Patch Fixed typos and trailing spaces.
Created attachment 251229 [details] Patch Replaced /**/ comments
Comment on attachment 251229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251229&action=review Quick review but nothing looks out of place. > Source/WebCore/ChangeLog:12 > + A controller is created at the time a ReadableJSStream is started and it is owned by it. > + The controller will be destroyed when the ReadableJSStream is also destroyed. Am I getting this right? -ReadableStreamJSSource creates the ReadableStreamController -It creates a JS wrapper for the controller. -You keep a strong reference to the wrapper to ensure that ReadableStreamController does not outlive ReadableStreamJSSource.
(In reply to comment #24) > Comment on attachment 251229 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251229&action=review > > Quick review but nothing looks out of place. Thanks. > > Source/WebCore/ChangeLog:12 > > + A controller is created at the time a ReadableJSStream is started and it is owned by it. > > + The controller will be destroyed when the ReadableJSStream is also destroyed. > > Am I getting this right? > -ReadableStreamJSSource creates the ReadableStreamController > -It creates a JS wrapper for the controller. > -You keep a strong reference to the wrapper to ensure that > ReadableStreamController does not outlive ReadableStreamJSSource. That is it, maybe the change log could be further improved when landing. As you spotted previously, ReadableStreamController may outlive ReadableStreamJSSource if the JS keeps a reference to it. In that case, as seen in ReadableStreamJSSource destructor, ReadableStreamController stream is set to null, which makes the controller returns exception for any function call. This could also be added in the change log.
Created attachment 251313 [details] Patch Fixed changelog according to comments
Comment on attachment 251313 [details] Patch Landing based on previous r+
Comment on attachment 251313 [details] Patch Clearing flags on attachment: 251313 Committed r183107: <http://trac.webkit.org/changeset/183107>
All reviewed patches have been landed. Closing bug.