[Streams API] Construct a writable stream
Created attachment 263573 [details] Patch This patch constructs a WritableStream. For that it introduces the WritableStreamInternals.
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.
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?
Created attachment 263665 [details] Patch for landing
(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.
Comment on attachment 263665 [details] Patch for landing Clearing flags on attachment: 263665 Committed r191383: <http://trac.webkit.org/changeset/191383>
All reviewed patches have been landed. Closing bug.
(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.
(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.