As per the spec, close and enqueue are aligned with error method. They should throw if stream is not readable.
Created attachment 283889 [details] Patch
Comment on attachment 283889 [details] Patch Attachment 283889 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1700405 New failing tests: streams/reference-implementation/readable-stream-templated.html streams/reference-implementation/pipe-to-options.html
Created attachment 283892 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 283889 [details] Patch Attachment 283889 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1700407 New failing tests: streams/reference-implementation/readable-stream-templated.html streams/reference-implementation/pipe-to-options.html
Created attachment 283893 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.5
Created attachment 283984 [details] Patch
(In reply to comment #4) > Comment on attachment 283889 [details] > Patch > > Attachment 283889 [details] did not pass ios-sim-ews (ios-simulator-wk2): > Output: http://webkit-queues.webkit.org/results/1700407 > > New failing tests: > streams/reference-implementation/readable-stream-templated.html > streams/reference-implementation/pipe-to-options.html Fixed failures for these tests. Note that pipeTo is still in flux. These tests should probably be tightened/updated once pipeTo is finalised.
Comment on attachment 283984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283984&action=review > LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:6 > +CONSOLE MESSAGE: line 405: TypeError: ReadableStream is not readable > +CONSOLE MESSAGE: line 405: TypeError: ReadableStream is not readable > +CONSOLE MESSAGE: line 406: TypeError: ReadableStream is not readable > +CONSOLE MESSAGE: line 406: TypeError: ReadableStream is not readable > + > +Harness Error (FAIL), message = TypeError: ReadableStream is not readable Why does this happen? Can you point the affected tests?
(In reply to comment #8) > Comment on attachment 283984 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=283984&action=review > > > LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:6 > > +CONSOLE MESSAGE: line 405: TypeError: ReadableStream is not readable > > +CONSOLE MESSAGE: line 405: TypeError: ReadableStream is not readable > > +CONSOLE MESSAGE: line 406: TypeError: ReadableStream is not readable > > +CONSOLE MESSAGE: line 406: TypeError: ReadableStream is not readable > > + > > +Harness Error (FAIL), message = TypeError: ReadableStream is not readable > > Why does this happen? Can you point the affected tests? This happens twice for the same templated test that tries to asynchronously enqueue and then close the stream (two chunks enqueued async, then closed). But pipeTo already closed the stream in two cases, hence why enqueue and close are throwing for those configurations. I would probably be nicer to add some assert_throw but I am not sure this is worth it. These tests with these console messages exercice well whether the stream is closed or not.
Comment on attachment 283984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283984&action=review >>> LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:6 >>> +Harness Error (FAIL), message = TypeError: ReadableStream is not readable >> >> Why does this happen? Can you point the affected tests? > > This happens twice for the same templated test that tries to asynchronously enqueue and then close the stream (two chunks enqueued async, then closed). > > But pipeTo already closed the stream in two cases, hence why enqueue and close are throwing for those configurations. > I would probably be nicer to add some assert_throw but I am not sure this is worth it. > These tests with these console messages exercice well whether the stream is closed or not. Sorry, I repent the r+ :), the problem with this output is that you don't know which test is causing that output, I think that needs to be changed.
I'll update the test according what repo test then. Might be good to move these to W3C WPT repo also. Do you remember why they were not migrated?
(In reply to comment #11) > I'll update the test according what repo test then. Good. > Might be good to move these to W3C WPT repo also. It would be good. > Do you remember why they were not migrated? IIRC, Domenic only migrated some tests and if some of them were not finished it was either because of lack of time (I think he began with "stable" RS features and left WR and friends out) or because things were not stable enough.
Created attachment 284013 [details] Rebasing tests according whatwg
Comment on attachment 284013 [details] Rebasing tests according whatwg View in context: https://bugs.webkit.org/attachment.cgi?id=284013&action=review > LayoutTests/streams/reference-implementation/readable-stream-templated.html:407 > + if (!this._cancelled) > + c.enqueue(chunks[0]); Though it might be interesting to check that the exception is being thrown here, this is checked in other places so it is not strictly necessary. Feel free to add it if you feel like.
(In reply to comment #14) > Comment on attachment 284013 [details] > Rebasing tests according whatwg > > View in context: > https://bugs.webkit.org/attachment.cgi?id=284013&action=review > > > LayoutTests/streams/reference-implementation/readable-stream-templated.html:407 > > + if (!this._cancelled) > > + c.enqueue(chunks[0]); > > Though it might be interesting to check that the exception is being thrown > here, this is checked in other places so it is not strictly necessary. Feel > free to add it if you feel like. Thanks for the review. I prefer keeping close to the original tests here.
Comment on attachment 284013 [details] Rebasing tests according whatwg Clearing flags on attachment: 284013 Committed r203411: <http://trac.webkit.org/changeset/203411>
All reviewed patches have been landed. Closing bug.