Bug 144907 - [Streams API] Delegate ReadableStreamReader reference counting to ReadableStream
Summary: [Streams API] Delegate ReadableStreamReader reference counting to ReadableStream
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-12 02:35 PDT by youenn fablet
Modified: 2015-05-17 11:44 PDT (History)
4 users (show)

See Also:

Attachments
WIP (16.68 KB, patch)
2015-05-12 03:20 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch (17.74 KB, patch)
2015-05-12 06:06 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch (18.18 KB, patch)
2015-05-12 11:48 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2015-05-12 02:35:36 PDT 
The current implementation handles reader and stream as related but independent entities.
This causes some complexity and some bugs to maintain their state and link correctly.
It might be simpler to adapt the ReadableStreamController link for the reader.
Comment 1 youenn fablet 2015-05-12 03:20:51 PDT 
Created attachment 252953 [details]
WIP
Comment 2 youenn fablet 2015-05-12 03:21:32 PDT 
(In reply to comment #1)
> Created attachment 252953 [details]
> WIP

WIP, does not contain moving closedpromise callback handling to ReadableStream, which would be the next logical step.
Comment 3 youenn fablet 2015-05-12 06:06:01 PDT 
Created attachment 252958 [details]
Patch
Comment 4 youenn fablet 2015-05-12 11:48:56 PDT 
Created attachment 252973 [details]
Patch
Comment 5 youenn fablet 2015-05-12 11:57:29 PDT 
Comment on attachment 252973 [details]
Patch

This patch improves style over the previous patch.

It shows the benefit of removing RefCounted from ReadableStreamReader.
This removes code and at the end, there should be no need for a ReadableStreamReader.cpp, just like ReadableStreamController.

There are two downsides though:
- The ReadableStream will not be released until all readers, including released readers, are collected by the JS engine.
- The ReadableStream currently needs to keep track of all released readers.

Overall I think that this is an improvement, so r?
Comment 6 WebKit Commit Bot 2015-05-17 11:44:45 PDT 
Comment on attachment 252973 [details]
Patch

Clearing flags on attachment: 252973

Committed r184444: <http://trac.webkit.org/changeset/184444>
Comment 7 WebKit Commit Bot 2015-05-17 11:44:49 PDT 
All reviewed patches have been landed.  Closing bug.