Bug 150360 – [Streams API] Construct a writable stream

• Component: New Bugs
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

[Streams API] Construct a writable stream

[Xabier Rodríguez Calvar]

Created attachment 263573 [details]
Patch

This patch constructs a WritableStream. For that it introduces the WritableStreamInternals.

[Darin Adler]

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 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]

Created attachment 263665 [details]
Patch for landing

[Xabier Rodríguez Calvar]

(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 on attachment 263665 [details]
Patch for landing

Clearing flags on attachment: 263665

Committed r191383: <http://trac.webkit.org/changeset/191383>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

(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]

(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.