Bug 152535 – [Streams API] Refactor builtin internals to prepare support for streams API in worker

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

To prepare bug 152066, it may be good to refactor the code to:
- enable exposing private builtins to worker
- split to-be-generated code from regular code

[youenn fablet]

Created attachment 267851 [details]
Patch

[Alex Christensen]

Comment on attachment 267851 [details]
Patch

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

> Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.cpp:64
> +#ifndef SKIP_UNUSED_PARAM

What's this? Shouldn't we have #else or #if !ENABLE(STREAMS_API) && !ENABLE(MEDIA_STREAM)?

[Xabier Rodríguez Calvar]

Comment on attachment 267851 [details]
Patch

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

I think somebody with more knowledge of JSC than me should review this though.

> Source/WebCore/bindings/js/JSDOMGlobalObject.h:82
> +        void addBuiltinGlobals(JSC::VM&);

Can't this be private?

> Source/WebCore/bindings/js/JSDOMGlobalObject.h:84
> +        friend void JSBuiltinInternalFunctions::initialize(JSDOMGlobalObject&, JSC::VM&);
> +        friend void JSBuiltinInternalObjects::initialize(JSDOMGlobalObject&, JSC::VM&);

I don't really like using friend as it usually shows that the design can be improved. Considering that GlobalPropertyInfo is just a structure it wouldn't make too much harm to make it public and then you only have to solve the issue with addStaticGlobals. That can be quickly solved by adding another method to JSDOMGlobalObject. The issue with this is that every class has the chance to add more static globals, which I guess it can be a problem too. I don't have a strong opinion, just wanted to understand the pros and cons.

> Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.cpp:142
> +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamClosedPrivateName(), jsNumber(1), DontDelete | ReadOnly),
> +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamClosingPrivateName(), jsNumber(2), DontDelete | ReadOnly),
> +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamErroredPrivateName(), jsNumber(3), DontDelete | ReadOnly),
> +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamReadablePrivateName(), jsNumber(4), DontDelete | ReadOnly),
> +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamWaitingPrivateName(), jsNumber(5), DontDelete | ReadOnly),
> +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamWritablePrivateName(), jsNumber(6), DontDelete | ReadOnly),

I think it is the moment to use once and for all an enum here.

[youenn fablet]

Thanks for the feedback.

Note that this patch is a trimmed down version (no IDL change) of rolled out patch for bug 152066. It also ensures ReadableStream private constructors do not get GCed.

Some additional comments below.

> > Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.cpp:64
> > +#ifndef SKIP_UNUSED_PARAM
> 
> What's this? Shouldn't we have #else or #if !ENABLE(STREAMS_API) &&
> !ENABLE(MEDIA_STREAM)?

This is indeed equivalent.
It was thought more elegant to define SKIP_UNUSED_PARAM once and reuse it
than to add the multi-if statement several times.
This patch keeps the same strategy.

An alternative might be to define a dummy class definition for the SKIP_UNUSED_PARAM case.

> > Source/WebCore/bindings/js/JSDOMGlobalObject.h:82
> > +        void addBuiltinGlobals(JSC::VM&);
> 
> Can't this be private?

Good spot, I will fix that.

> > Source/WebCore/bindings/js/JSDOMGlobalObject.h:84
> > +        friend void JSBuiltinInternalFunctions::initialize(JSDOMGlobalObject&, JSC::VM&);
> > +        friend void JSBuiltinInternalObjects::initialize(JSDOMGlobalObject&, JSC::VM&);
> 
> I don't really like using friend as it usually shows that the design can be
> improved. Considering that GlobalPropertyInfo is just a structure it
> wouldn't make too much harm to make it public and then you only have to
> solve the issue with addStaticGlobals. That can be quickly solved by adding
> another method to JSDOMGlobalObject. The issue with this is that every class
> has the chance to add more static globals, which I guess it can be a problem
> too. I don't have a strong opinion, just wanted to understand the pros and
> cons.

friend usage seems fine here as we basically split code from a single class in severals, as some parts can be best generated automatically.(JSBuiltinInternalFunctions).
For the other one, JSBuiltinInternalObjects purpose is purely syntactic. It could also stay inlined in JSDOMGlobalObject.
I think it helps keep the code a bit cleaner and easier to read. But I do not have a strong opinion.

JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamClosedPrivateName(), jsNumber(1), DontDelete | ReadOnly),
> > +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamClosingPrivateName(), jsNumber(2), DontDelete | ReadOnly),
> > +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamErroredPrivateName(), jsNumber(3), DontDelete | ReadOnly),
> > +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamReadablePrivateName(), jsNumber(4), DontDelete | ReadOnly),
> > +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamWaitingPrivateName(), jsNumber(5), DontDelete | ReadOnly),
> > +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamWritablePrivateName(), jsNumber(6), DontDelete | ReadOnly),
> 
> I think it is the moment to use once and for all an enum here.

Can you elaborate on the benefit?

[youenn fablet]

Created attachment 268191 [details]
Fixing private and removing one friend

[Xabier Rodríguez Calvar]

(In reply to comment #4)
> JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().
> streamClosedPrivateName(), jsNumber(1), DontDelete | ReadOnly),
> > > +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamClosingPrivateName(), jsNumber(2), DontDelete | ReadOnly),
> > > +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamErroredPrivateName(), jsNumber(3), DontDelete | ReadOnly),
> > > +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamReadablePrivateName(), jsNumber(4), DontDelete | ReadOnly),
> > > +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamWaitingPrivateName(), jsNumber(5), DontDelete | ReadOnly),
> > > +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().streamWritablePrivateName(), jsNumber(6), DontDelete | ReadOnly),
> > 
> > I think it is the moment to use once and for all an enum here.
> 
> Can you elaborate on the benefit?

We are creating an "enumeration" in JS by assigning a name to a value. I think that value should be taken from an enumeration in C++ mimicing the same values.

The benefits I see is readability and I think it is a bit less error prone.

[youenn fablet]

> We are creating an "enumeration" in JS by assigning a name to a value. I
> think that value should be taken from an enumeration in C++ mimicing the
> same values.
> 
> The benefits I see is readability and I think it is a bit less error prone.

I do not have any strong opinion on this.
I think the way it is now is a tad clearer, but difference is rather small.
Let's see what others think.

Any other comment/review?

[youenn fablet]

Ping review?

[WebKit Commit Bot]

Comment on attachment 268191 [details]
Fixing private and removing one friend

Clearing flags on attachment: 268191

Committed r194960: <http://trac.webkit.org/changeset/194960>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.