Implement error() method of ReadableByteStreamController
Created attachment 293661 [details] Patch
Comment on attachment 293661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293661&action=review > Source/WebCore/Modules/streams/ReadableByteStreamController.js:41 > + @makeThisTypeError("ReadableByteStreamController", "error"); Should we add a test for this one as well? Maybe it can be covered by WebIDL based tests (see wpt for more info). > Source/WebCore/Modules/streams/ReadableByteStreamController.js:43 > + const stream = this.@controlledReadableStream; No need to define stream since it is used only once. > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:87 > + return @isObject(controller) && !!controller.@underlyingByteSource; This is the same test mechanism as isReadableStreamController. We might not need to restate the same comment here, just forward to ReadableStreamInternals.js. Also maybe we should just do something like controller instanceof @ReadableStreamDefaultController, even though this departs from the spec. It should be equivalent at least currently since all steams/controllers are built using these constructors. > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:94 > + const stream = controller.@controlledReadableStream; stream is used only once in an assert, just better to not define it. > Source/WebCore/Modules/streams/ReadableStream.js:63 > + @throwTypeError("ReadableByteStreamController could not be created: " + e.message); Isn't there a risk that the try/catch error will change the error from range error to type error? If so, shouldn't we do something like: let readableByteStreamControllerConstructor; try { readableByteStreamControllerConstructor = @ReadableByteStreamController; } catch (e) { //not implemented error } this.@readableStreamController = new readableByteStreamControllerConstructor(this, underlyingSource, strategy.highWaterMark);
Created attachment 293670 [details] Patch
Created attachment 293674 [details] Patch
(In reply to comment #2) > Comment on attachment 293661 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293661&action=review > > > Source/WebCore/Modules/streams/ReadableByteStreamController.js:41 > > + @makeThisTypeError("ReadableByteStreamController", "error"); > > Should we add a test for this one as well? > Maybe it can be covered by WebIDL based tests (see wpt for more info). I'm not sure how this could be tested. On the other hand, I was planning to add a test for the following test (line 43, check whether stream is in readable state) after implementing close(). > > Source/WebCore/Modules/streams/ReadableByteStreamController.js:43 > > + const stream = this.@controlledReadableStream; > > No need to define stream since it is used only once. I removed it (and did the same in ReadableStreamDefaultController.js). > > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:87 > > + return @isObject(controller) && !!controller.@underlyingByteSource; > > This is the same test mechanism as isReadableStreamController. We might not > need to restate the same comment here, just forward to > ReadableStreamInternals.js. > Also maybe we should just do something like controller instanceof > @ReadableStreamDefaultController, even though this departs from the spec. > It should be equivalent at least currently since all steams/controllers are > built using these constructors. I tried using "instanceof", but it results in a TypeError (instanceof called on an object with an invalid prototype property). Therefore I kept the same code, but modified the comment to point to isReadableStreamDefaultController. > > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:94 > > + const stream = controller.@controlledReadableStream; > > stream is used only once in an assert, just better to not define it. Removed. > > Source/WebCore/Modules/streams/ReadableStream.js:63 > > + @throwTypeError("ReadableByteStreamController could not be created: " + e.message); > > Isn't there a risk that the try/catch error will change the error from range > error to type error? > If so, shouldn't we do something like: > let readableByteStreamControllerConstructor; > try { > readableByteStreamControllerConstructor = @ReadableByteStreamController; > } catch (e) { > //not implemented error > } > this.@readableStreamController = new > readableByteStreamControllerConstructor(this, underlyingSource, > strategy.highWaterMark); Right, this is cleaner. In the new patch, I implemented something similar except that I define readableByteStreamControllerConstructor only inside the try statement (and still use “new @ReadableByteStreamController()” after the try/catch). In the end, it will be possible to remove the try/catch without modifying the rest of the code.
(In reply to comment #5) > (In reply to comment #2) > > Comment on attachment 293661 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=293661&action=review > > > > > Source/WebCore/Modules/streams/ReadableByteStreamController.js:41 > > > + @makeThisTypeError("ReadableByteStreamController", "error"); > > > > Should we add a test for this one as well? > > Maybe it can be covered by WebIDL based tests (see wpt for more info). > > I'm not sure how this could be tested. On the other hand, I was planning to > add a test for the following test (line 43, check whether stream is in > readable state) after implementing close(). You can try to use https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/Objets_globaux/Function/apply with a this object of a different type and print the exception message or check at least that it is a TypeError. Please add something like LayoutTests/imported/w3c/web-platform-tests/streams/readable-streams/readable-stream-reader.js or LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-idl.html (WebIDL based)
Created attachment 293875 [details] Patch
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #2) > > > Comment on attachment 293661 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=293661&action=review > > > > > > > Source/WebCore/Modules/streams/ReadableByteStreamController.js:41 > > > > + @makeThisTypeError("ReadableByteStreamController", "error"); > > > > > > Should we add a test for this one as well? > > > Maybe it can be covered by WebIDL based tests (see wpt for more info). > > > > I'm not sure how this could be tested. On the other hand, I was planning to > > add a test for the following test (line 43, check whether stream is in > > readable state) after implementing close(). > > You can try to use > https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/ > Objets_globaux/Function/apply with a this object of a different type and > print the exception message or check at least that it is a TypeError. Thanks, I added such a test. > Please add something like > LayoutTests/imported/w3c/web-platform-tests/streams/readable-streams/ > readable-stream-reader.js or > LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-idl. > html (WebIDL based) I tried both approaches but had troubles with WebIDL-based tests. So I added corresponding tests in readable-byte-stream-controller.js. All behave as expected.
Comment on attachment 293875 [details] Patch Clearing flags on attachment: 293875 Committed r208382: <http://trac.webkit.org/changeset/208382>
All reviewed patches have been landed. Closing bug.
Reverted r208382 for reason: This change appears to have caused 3 SerializedCryptoKeyWrapTest API tests to fail on macOS. Committed r208422: <http://trac.webkit.org/changeset/208422>
(In reply to comment #11) > Reverted r208382 for reason: > > This change appears to have caused 3 SerializedCryptoKeyWrapTest API tests > to fail on macOS. > > Committed r208422: <http://trac.webkit.org/changeset/208422> Link to failures: https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/1231
The API tests are still failing after the rollout. https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/19305 I will roll the patch back in later today unless someone else beats me to it.
Reverted r208422 for reason: Roll r208382 back in since it was not responsible for the API test failures seen on macOS. Committed r208434: <http://trac.webkit.org/changeset/208434>