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
youenn fablet
2015-12-23 12:27:39 PST
Created attachment 267851 [details]
Patch
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 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. 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? Created attachment 268191 [details]
Fixing private and removing one friend
(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. > 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?
Ping review? Comment on attachment 268191 [details] Fixing private and removing one friend Clearing flags on attachment: 268191 Committed r194960: <http://trac.webkit.org/changeset/194960> All reviewed patches have been landed. Closing bug. |