Bug 150589 – [Streams API] Add write method to writable stream

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Xabier Rodríguez Calvar]

[Streams API] Add write method to writable stream

[Xabier Rodríguez Calvar]

Created attachment 264129 [details]
Patch

Writable stream functionality should be complete now

[youenn fablet]

Comment on attachment 264129 [details]
Patch

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

Some nits

> Source/WebCore/Modules/streams/WritableStream.js:117
> +    if (this.@state === "closed" || this.@state === "closing")

Can we use integer comparison like for RS and Promise?

> Source/WebCore/Modules/streams/WritableStream.js:124
> +    // assert(this.@state === "writable" || this.@state === "waiting" || this.@state === undefined);

Why undefined?

> Source/WebCore/Modules/streams/WritableStream.js:129
> +            chunkSize = [e-mail].@apply(undefined, [chunk]);

Use @call instead?
This may also apply to other WritableStream*.js places

> Source/WebCore/Modules/streams/WritableStream.js:131
> +            @errorWritableStream.@apply(this, [e]);

Ditto

> Source/WebCore/Modules/streams/WritableStream.js:139
> +        @enqueueValueWithSize(this.@queue, writeRecord, chunkSize);

Maybe inline writeRecord definition.

> Source/WebCore/Modules/streams/WritableStream.js:141
> +        @errorWritableStream.@apply(this, [e]);

@apply -> @call

[Xabier Rodríguez Calvar]

(In reply to comment #2)
> > Source/WebCore/Modules/streams/WritableStream.js:117
> > +    if (this.@state === "closed" || this.@state === "closing")
> 
> Can we use integer comparison like for RS and Promise?

This is interesting. The difference between RS and WS is that with WS the state is returned by the state attribute as a string. That's why I didn't use the integer comparison. If I did I'd still have to covert it when returning it in the attribute.

I'll land this as it is now as it is and do this in a follow up patch if we think it is worth. TBH, I don't know which option can be better, probably the integer comparison.

> > Source/WebCore/Modules/streams/WritableStream.js:124
> > +    // assert(this.@state === "writable" || this.@state === "waiting" || this.@state === undefined);
> 
> Why undefined?

This is even more interesting. It seems that I didn't see until now the initialization of this@[[state]] = "writing" in the spec at the initialization and hence I didn't include it. This caused that in some we were hitting an undefined state.

I'll fix this in a follow up, either with an entire rework of the state to make it integer or on its own.

> > Source/WebCore/Modules/streams/WritableStream.js:129
> > +            chunkSize = [e-mail].@apply(undefined, [chunk]);
> 
> Use @call instead?
> This may also apply to other WritableStream*.js places
> 
> > Source/WebCore/Modules/streams/WritableStream.js:131
> > +            @errorWritableStream.@apply(this, [e]);
> 
> Ditto
> 
> > Source/WebCore/Modules/streams/WritableStream.js:141
> > +        @errorWritableStream.@apply(this, [e]);
> 
> @apply -> @call

On it.

> > Source/WebCore/Modules/streams/WritableStream.js:139
> > +        @enqueueValueWithSize(this.@queue, writeRecord, chunkSize);
> 
> Maybe inline writeRecord definition.

I wrote it like this to be more literal to the spec, but I guess we can get it changed.

[Xabier Rodríguez Calvar]

Created attachment 264209 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 264209 [details]
Patch for landing

Clearing flags on attachment: 264209

Committed r191669: <http://trac.webkit.org/changeset/191669>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.