Bug 151497 - [Streams API] pull function of tee should call readFromReadableStreamReader directly
Summary: [Streams API] pull function of tee should call readFromReadableStreamReader d...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-20 09:32 PST by youenn fablet
Modified: 2015-12-01 03:02 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.66 KB, patch)
2015-11-20 09:34 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (4.47 KB, patch)
2015-12-01 01:42 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (4.45 KB, patch)
2015-12-01 02:20 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2015-11-20 09:32:50 PST
pull function of tee should call readFromReadableStreamReader directly.
Currently it calls ReadableStream.read() which is suboptimal.
Comment 1 youenn fablet 2015-11-20 09:34:58 PST
Created attachment 265960 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2015-11-23 01:43:56 PST
Comment on attachment 265960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265960&action=review

> Source/WebCore/Modules/streams/ReadableStreamInternals.js:-119
> -        @Promise.prototype.@then.@call(reader.read(), function(result) {

I was kind of puzzled when I read this the first time so I guess it makes quite a lot of sense to change it.
Comment 3 youenn fablet 2015-11-23 02:58:46 PST
Comment on attachment 265960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265960&action=review

>> Source/WebCore/Modules/streams/ReadableStreamInternals.js:-119
>> -        @Promise.prototype.@then.@call(reader.read(), function(result) {
> 
> I was kind of puzzled when I read this the first time so I guess it makes quite a lot of sense to change it.

In that specific case, it is safe to write (and much easier to read): @readFromReadableStreamReader(reader).@then(function ...);
It would be good to write it that way for most (if not all) parts of streams API code to keep consistency and improve readability.

That may mean some refactoring in invokeOrNoop and the likes to ensure @then is available.
All these changes might best be adressed as a follow-up patch.
Comment 4 youenn fablet 2015-12-01 01:42:51 PST
Created attachment 266348 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2015-12-01 01:44:41 PST
Comment on attachment 266348 [details]
Patch for landing

Rejecting attachment 266348 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 266348, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
with fuzz 1.
patching file LayoutTests/streams/streams-promises-expected.txt
Hunk #1 FAILED at 8.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/streams/streams-promises-expected.txt.rej
patching file LayoutTests/streams/streams-promises.html
Hunk #1 FAILED at 125.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/streams/streams-promises.html.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/500863
Comment 6 youenn fablet 2015-12-01 01:46:06 PST
(In reply to comment #4)
> Created attachment 266348 [details]
> Patch for landing

Thanks for the r+
Along the lines of other related patches, I added a test to ensure we no longer use ReadableStreamReader.read().
It uses promise_test as a way to execute sequentially an otherwise async_test.
Comment 7 WebKit Commit Bot 2015-12-01 02:16:47 PST
Comment on attachment 266348 [details]
Patch for landing

Rejecting attachment 266348 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 266348, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/500949
Comment 8 youenn fablet 2015-12-01 02:20:26 PST
Created attachment 266350 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2015-12-01 03:02:44 PST
Comment on attachment 266350 [details]
Patch for landing

Clearing flags on attachment: 266350

Committed r192879: <http://trac.webkit.org/changeset/192879>
Comment 10 WebKit Commit Bot 2015-12-01 03:02:48 PST
All reviewed patches have been landed.  Closing bug.