Bug 152535

Summary: [Streams API] Refactor builtin internals to prepare support for streams API in worker
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, commit-queue, darin
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 152066    
Attachments:
Description Flags
Patch
none
Fixing private and removing one friend none

Description youenn fablet 2015-12-23 12:27:39 PST
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
Comment 1 youenn fablet 2015-12-23 12:50:06 PST
Created attachment 267851 [details]
Patch
Comment 2 Alex Christensen 2015-12-26 17:48:26 PST
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)?
Comment 3 Xabier Rodríguez Calvar 2015-12-28 06:34:44 PST
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.
Comment 4 youenn fablet 2016-01-02 04:51:05 PST
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?
Comment 5 youenn fablet 2016-01-04 08:11:41 PST
Created attachment 268191 [details]
Fixing private and removing one friend
Comment 6 Xabier Rodríguez Calvar 2016-01-04 08:46:27 PST
(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.
Comment 7 youenn fablet 2016-01-05 01:04:53 PST
> 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?
Comment 8 youenn fablet 2016-01-12 07:00:34 PST
Ping review?
Comment 9 WebKit Commit Bot 2016-01-13 01:54:39 PST
Comment on attachment 268191 [details]
Fixing private and removing one friend

Clearing flags on attachment: 268191

Committed r194960: <http://trac.webkit.org/changeset/194960>
Comment 10 WebKit Commit Bot 2016-01-13 01:54:42 PST
All reviewed patches have been landed.  Closing bug.