(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.
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
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
(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.
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.
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.
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
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
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
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.
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
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
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
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.
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
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
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.
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.
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.
2014-11-24 01:19 PST, youenn fablet
2014-11-24 01:20 PST, youenn fablet
2014-12-10 07:01 PST, youenn fablet
2014-12-10 07:45 PST, Build Bot
2014-12-10 08:19 PST, Build Bot
2015-08-21 03:40 PDT, youenn fablet
2016-03-21 11:51 PDT, youenn fablet
2016-03-24 09:13 PDT, youenn fablet
2016-04-05 02:05 PDT, youenn fablet
2016-04-05 03:01 PDT, Build Bot
2016-04-05 03:16 PDT, Build Bot
2016-04-05 03:24 PDT, Build Bot
2016-04-05 06:11 PDT, youenn fablet
2016-04-05 07:06 PDT, Build Bot
2016-04-05 07:11 PDT, Build Bot
2016-04-05 07:26 PDT, Build Bot
2016-04-05 15:26 PDT, youenn fablet
2016-04-05 16:22 PDT, Build Bot
2016-04-05 16:26 PDT, Build Bot
2016-04-06 02:40 PDT, youenn fablet
2016-04-06 06:19 PDT, youenn fablet
2016-04-17 09:37 PDT, youenn fablet