Bug 150667 – [Streams API] Turn WS states into integers and fix state initialization

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

[Xabier Rodríguez Calvar]

[Streams API] Turn WS states into integers and fix state initialization

[Xabier Rodríguez Calvar]

Created attachment 264311 [details]
Patch

I wasn't sure if I should rework the rs states or keep them separated. I finally decided to go got reworking them though I don't have a string opinion about it. Turned ws states into integer values and also fixed the state not being initialized.

[youenn fablet]

Comment on attachment 264311 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableStreamInternals.js:-329
> -    // TODO: ASSERT(stream.@state !== @readableStreamErrored);

If we should use FIXME, maybe change the ones that got changed accordingly

> Source/WebCore/Modules/streams/WritableStream.js:195
> +    }

Might want to rewrite this with return and assert(false)

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:95
> +        GlobalPropertyInfo(static_cast<JSVMClientData*>(vm.clientData)->builtinNames().streamWritablePrivateName(), jsNumber(6), DontDelete | ReadOnly),

Somehow we should automate this private enumeration step.
That said, this seems good enough

[Xabier Rodríguez Calvar]

(In reply to comment #2)
> > Source/WebCore/Modules/streams/ReadableStreamInternals.js:-329
> > -    // TODO: ASSERT(stream.@state !== @readableStreamErrored);
> 
> If we should use FIXME, maybe change the ones that got changed accordingly

I'll change the ones that appear in the patch, but I was planning to do a wider style change so I'll fix the others later.

> > Source/WebCore/Modules/streams/WritableStream.js:195
> > +    }
> 
> Might want to rewrite this with return and assert(false)

I thought of that too, but I liked it more this because assert(false) seems dirty. I could do write assert_not_reached. Please, let me know if you prefer to leave it as it is now or an assert_not_reached.

> > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:95
> > +        GlobalPropertyInfo(static_cast<JSVMClientData*>(vm.clientData)->builtinNames().streamWritablePrivateName(), jsNumber(6), DontDelete | ReadOnly),
> 
> Somehow we should automate this private enumeration step.
> That said, this seems good enough

I thought of that too when I was touching this.

[Xabier Rodríguez Calvar]

Created attachment 264319 [details]
Patch for landing

[Xabier Rodríguez Calvar]

(In reply to comment #3)
> > If we should use FIXME, maybe change the ones that got changed accordingly
> 
> I'll change the ones that appear in the patch, but I was planning to do a
> wider style change so I'll fix the others later.

Changed the ones affected and things around for coherence.

> > > Source/WebCore/Modules/streams/WritableStream.js:195
> > > +    }
> > 
> > Might want to rewrite this with return and assert(false)
> 
> I thought of that too, but I liked it more this because assert(false) seems
> dirty. I could do write assert_not_reached. Please, let me know if you
> prefer to leave it as it is now or an assert_not_reached.

Changed to return with assert_not_reached.

[WebKit Commit Bot]

Comment on attachment 264319 [details]
Patch for landing

Clearing flags on attachment: 264319

Committed r191730: <http://trac.webkit.org/changeset/191730>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.