Created attachment 242148 [details] WIP

Created attachment 242149 [details] WIP

(In reply to comment #2) > Created attachment 242149 [details] > WIP This patch illustrates how ReadableStream (as implemented in latest WIP patch from bug 138967) can be instantiated from XHR response.

This depends on bug 138967, as this patch can't live without the other.

Created attachment 243013 [details] Patch

(In reply to comment #5) > Created attachment 243013 [details] > Patch Patch updates XHR integration according the latest ReadableStream API.

Comment on attachment 243013 [details] Patch Attachment 243013 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5988914837848064 New failing tests: http/tests/xmlhttprequest/streams/streams-read-api-closed.html http/tests/xmlhttprequest/streams/streams-read-api.html fast/xmlhttprequest/xmlhttprequest-responsetype-stream.html http/tests/xmlhttprequest/streams/streams-read.html http/tests/xmlhttprequest/streams/streams-read-api-cancelled.html

Created attachment 243020 [details] Archive of layout-test-results from ews101 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Comment on attachment 243013 [details] Patch Attachment 243013 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5859343190720512 New failing tests: http/tests/xmlhttprequest/streams/streams-read-api-closed.html http/tests/xmlhttprequest/streams/streams-read-api.html fast/xmlhttprequest/xmlhttprequest-responsetype-stream.html http/tests/xmlhttprequest/streams/streams-read.html http/tests/xmlhttprequest/streams/streams-read-api-cancelled.html

Created attachment 243024 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Comment on attachment 243013 [details] Patch Was not meant to be r?

Created attachment 259598 [details] Updated according ReadableStream implementation in JSC

(In reply to comment #12) > Created attachment 259598 [details] > Updated according ReadableStream implementation in JSC This is a mock-up version of how native ReadableStream sources could be implemented, not a real patch but it gives an idea. Basically, native sources would have an IDL that would implement the ReadableStream source API (start, pull and cancel). Similarly to promises, a WebCore wrapper around controller is introduced to more easily enqueue and error the streams with non-JS objects. Controller is kept as a cached attribute as this is not observable from JS scripts and it allows not using JSC::Strong.

Created attachment 274614 [details] WIP

Created attachment 274835 [details] Rebasing

(In reply to comment #15) > Created attachment 274835 [details] > Rebasing Patch probably needs more tests. Early feedback most welcome though.

Comment on attachment 274835 [details] Rebasing View in context: https://bugs.webkit.org/attachment.cgi?id=274835&action=review Just a quick glance: > Source/WebCore/Modules/fetch/FetchBody.cpp:191 This is what a switch statement is for. > Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:52 > +void FetchBodyOwner::stopBlobLoading() This change doesn't seem useful. > Source/WebCore/Modules/fetch/FetchLoader.cpp:107 > + RefPtr<SharedBuffer> data = WTFMove(m_data); No need for a named variable here

Attachment 274835 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/fetch/FetchResponse.h:122: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:43: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 275650 [details] Rebased and more tests

Attachment 275650 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/fetch/FetchResponseSource.h:41: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:45: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 275650 [details] Rebased and more tests Attachment 275650 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1102746 New failing tests: imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed.html

Created attachment 275653 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 275650 [details] Rebased and more tests Attachment 275650 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1102760 New failing tests: imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed.html

Created attachment 275654 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 275650 [details] Rebased and more tests Attachment 275650 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1102763 New failing tests: imported/w3c/web-platform-tests/fetch/api/response/response-cancel-stream.html imported/w3c/web-platform-tests/fetch/api/basic/stream-response.html

Created attachment 275656 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 275657 [details] Adding conditional compilation and fixing tests

Attachment 275657 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/fetch/FetchResponseSource.h:41: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:45: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 275657 [details] Adding conditional compilation and fixing tests Attachment 275657 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1103668 New failing tests: imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed-4.html

Created attachment 275661 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 275657 [details] Adding conditional compilation and fixing tests Attachment 275657 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1103670 New failing tests: imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed-4.html

Created attachment 275664 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 275657 [details] Adding conditional compilation and fixing tests Attachment 275657 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1103723 New failing tests: imported/w3c/web-platform-tests/fetch/api/basic/stream-response.html

Created attachment 275666 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 275703 [details] Fixing stream-response.html

Attachment 275703 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/fetch/FetchResponseSource.h:41: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:45: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 275703 [details] Fixing stream-response.html Attachment 275703 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1105872 New failing tests: imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed-4.html

Created attachment 275712 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 275703 [details] Fixing stream-response.html Attachment 275703 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1105867 New failing tests: imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed-4.html

Created attachment 275713 [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.10.5

Created attachment 275765 [details] Trying to fix response-stream-disturbed-4.html

Attachment 275765 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/fetch/FetchResponseSource.h:41: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:45: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 275765 [details] Trying to fix response-stream-disturbed-4.html Considering that Fetch will need the Source and the ReadableStreamController, it might make more sense to split this patch into at least two, one for the RS and the other for Fetch code. Said this, I also think there were some changes in the spec lately that would be interesting to have a look at, just in case we are doing things that changed already, etc.

Created attachment 275772 [details] Patch

Attachment 275772 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:45: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 47 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 275772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275772&action=review > Source/WebCore/ChangeLog:11 > + A createReadableStream function is introduced to allow DOM classes createa ReadableStream. Nit: typo "createa" > Source/WebCore/Modules/fetch/FetchBody.cpp:159 > + source.enqueue(ArrayBuffer::tryCreate(data.data(), data.size())); Should this have a FIXME like that in BodyLoader::didReceiveData about what to do if ArrayBuffer::tryCreate returns null? > Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:209 > + m_readableStreamSource->enqueue(ArrayBuffer::tryCreate(data, size)); Ditto.

Comment on attachment 275772 [details] Patch Thanks for the feedback. >> Source/WebCore/Modules/fetch/FetchBody.cpp:159 >> + source.enqueue(ArrayBuffer::tryCreate(data.data(), data.size())); > > Should this have a FIXME like that in BodyLoader::didReceiveData about what to do if ArrayBuffer::tryCreate returns null? Yes, in that case, the stream will get errored and calling stream.close() afterwards will raise an exception. Either we should check it here or handle this at FetchResponseSource::close level. I will add a FIXME. The annoying thing is that I do not know how to write a test for this code path. >> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:209 >> + m_readableStreamSource->enqueue(ArrayBuffer::tryCreate(data, size)); > > Ditto. Right, we should add the same FIXME as for FetchResponse::BodyLoader::didReceiveData.

Comment on attachment 275772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275772&action=review The code looks good and it makes many tests pass, which is good. I can't say I can wrap my head completely around the design of these objects, though. > Source/WebCore/ChangeLog:8 > + This patch introduces ReadableStreamSource and ReadableStreamController which allow feeding a ReadableStream from DOM classes. I'm not sure about this design. It might be great, but a quick check shows Chromium has no such classes. Could you describe why you went with this approach? > Source/WebCore/Modules/fetch/FetchBody.cpp:164 > + case Type::Blob: > + owner.loadBlob(*m_blob, FetchLoader::Type::Stream); At least assert m_blob > Source/WebCore/bindings/js/ReadableStreamController.cpp:70 > + JSC::ExecState& state = *globalObject()->globalExec(); This is a lot of unchecked pointer dereferencing.

ReadableStream needs a JS source object with some of start, pull and cancel methods. Parameter passed to start and pull is a controller which has enqueue/error/close methods to control the stream state. The controller (JSReadableStreamController) is a JS built-in object. To help calling enqueue/close/error JS methods from dom classes, it is convenient to add a wrapper. This is the purpose of ReadableStreamController. It can also be used to query some stream internals (locked, disturbed). To implement the source, using IDL seems like the easiest choice. Currently ReadableStreamSource can handle any type of sources. Since fetch is push type, we could make it less general and probably more simple: no pull method, cancel returning void (sync cancel). In that case, ReadableStreamPushSource could be introduced instead. For fetch, the start promise is never resolved. This disables any call to pull. This also allows to enqueue/close/error at anytime. FetchResponse is marked as pending activity when stream is started until the stream gets closed, errored or canceled. I am not sure about chromium, I haven't checked its status lately. At some point streams were Dom/c++ based and thus directly tied to fetch. I think there were discussions to go to JS built-in but cannot say more than that.

Created attachment 276592 [details] After review

Attachment 276592 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:45: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 47 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #50) > Created attachment 276592 [details] > After review After thinking more about it, I removed the pull method from ReadableStreamSource since it is never called by FetchResponse. I also made cancel returning void since it is cancelled synchronously.

Comment on attachment 276592 [details] After review Clearing flags on attachment: 276592 Committed r199641: <http://trac.webkit.org/changeset/199641>

All reviewed patches have been landed. Closing bug.