RESOLVED FIXED 150360
[Streams API] Construct a writable stream
https://bugs.webkit.org/show_bug.cgi?id=150360
Summary [Streams API] Construct a writable stream
Xabier Rodríguez Calvar
Reported 2015-10-20 05:23:38 PDT
[Streams API] Construct a writable stream
Attachments
Patch (56.73 KB, patch)
2015-10-20 05:36 PDT, Xabier Rodríguez Calvar
no flags
Patch for landing (56.56 KB, patch)
2015-10-21 00:40 PDT, Xabier Rodríguez Calvar
no flags
Xabier Rodríguez Calvar
Comment 1 2015-10-20 05:36:15 PDT
Created attachment 263573 [details] Patch This patch constructs a WritableStream. For that it introduces the WritableStreamInternals.
Darin Adler
Comment 2 2015-10-20 09:06:02 PDT
Comment on attachment 263573 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263573&action=review > Source/WebCore/Modules/streams/WritableStream.js:41 > + if (strategy !== undefined && !@isObject(strategy)) I don’t see how strategy could be undefined here. I think the first half of this predicate is dead code.
youenn fablet
Comment 3 2015-10-20 12:05:36 PDT
Comment on attachment 263573 [details] Patch Great to see the first WS tests passing :) For ReadableStream, @state takes private symbol-based integer values like promise impl (@readableStreamClosed and so on). We might want to do the same for writable stream. A couple of style nits below. View in context: https://bugs.webkit.org/attachment.cgi?id=263573&action=review > Source/WebCore/Modules/streams/WritableStream.js:57 > + this.@startedPromise = Promise.resolve(startResult); Maybe inline startResult in this line? > Source/WebCore/Modules/streams/WritableStream.js:63 > + error(r); Can we just put error, instead of "function(r) { error(r); }"? > Source/WebCore/Modules/streams/WritableStreamInternals.js:35 > + return undefined; Just "return;" maybe? > Source/WebCore/Modules/streams/WritableStreamInternals.js:45 > + @resolveStreamsPromise(stream.@readyPromise, undefined); Maybe just "@resolveStreamsPromise(stream.@readyPromise);" ? > Source/WebCore/Modules/streams/WritableStreamInternals.js:48 > + return undefined; no need for this line? > Source/WebCore/Modules/streams/WritableStreamInternals.js:56 > + return undefined; Maybe "return;" ? > Source/WebCore/Modules/streams/WritableStreamInternals.js:67 > + return undefined; no need for this line?
Xabier Rodríguez Calvar
Comment 4 2015-10-21 00:40:18 PDT
Created attachment 263665 [details] Patch for landing
Xabier Rodríguez Calvar
Comment 5 2015-10-21 01:06:55 PDT
(In reply to comment #2) > > Source/WebCore/Modules/streams/WritableStream.js:41 > > + if (strategy !== undefined && !@isObject(strategy)) > > I don’t see how strategy could be undefined here. I think the first half of > this predicate is dead code. It seems to be because of the previous lines, yes. Landed it that change. (In reply to comment #3) Most of these things I did to be more literal to the spec. We could end up deciding against this, but I think it is ok to be literal in this cases, even when it goes against JS style conventions. > > Source/WebCore/Modules/streams/WritableStream.js:57 > > + this.@startedPromise = Promise.resolve(startResult); > > Maybe inline startResult in this line? I thought of that at the moment, but I decided to leave it as it is step 12 of the spec. > > Source/WebCore/Modules/streams/WritableStream.js:63 > > + error(r); > > Can we just put error, instead of "function(r) { error(r); }"? Yes, we could do that if we decide to touch code around there, not even worth a follow up patch. > > Source/WebCore/Modules/streams/WritableStreamInternals.js:35 > > + return undefined; > > Just "return;" maybe? Spec, step 1. > > Source/WebCore/Modules/streams/WritableStreamInternals.js:45 > > + @resolveStreamsPromise(stream.@readyPromise, undefined); > > Maybe just "@resolveStreamsPromise(stream.@readyPromise);" ? Spec, step 6b. > > Source/WebCore/Modules/streams/WritableStreamInternals.js:48 > > + return undefined; > > no need for this line? Spec, step 7. > > Source/WebCore/Modules/streams/WritableStreamInternals.js:56 > > + return undefined; > > Maybe "return;" ? Spec, step 1. > > Source/WebCore/Modules/streams/WritableStreamInternals.js:67 > > + return undefined; > > no need for this line? Spec, step 7.
WebKit Commit Bot
Comment 6 2015-10-21 01:35:22 PDT
Comment on attachment 263665 [details] Patch for landing Clearing flags on attachment: 263665 Committed r191383: <http://trac.webkit.org/changeset/191383>
WebKit Commit Bot
Comment 7 2015-10-21 01:35:26 PDT
All reviewed patches have been landed. Closing bug.
youenn fablet
Comment 8 2015-10-21 05:11:56 PDT
(In reply to comment #5) > Most of these things I did to be more literal to the spec. We could end up > deciding against this, but I think it is ok to be literal in this cases, > even when it goes against JS style conventions. For "@resolveStreamsPromise(stream.@readyPromise undefined);", I can understand the value somehow. In the other cases (mostly return statements), sticking with JS conventions seems more valuable to me. Streams API reference implementation is sticking with JS convention FWIW.
Xabier Rodríguez Calvar
Comment 9 2015-10-21 05:59:52 PDT
(In reply to comment #8) > For "@resolveStreamsPromise(stream.@readyPromise undefined);", I can > understand the value somehow. > > In the other cases (mostly return statements), sticking with JS conventions > seems more valuable to me. Streams API reference implementation is sticking > with JS convention FWIW. I can understand the value of both or none, but I wouldn't do a mix. We can keep these things in mind and "clean" the code afterwards if you think it is better.
Note You need to log in before you can comment on or make changes to this bug.