Bug 143276 – Clean up EnumerationMode to easily extend

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Spawned from Object.getOwnPropertySymbols patch https://bugs.webkit.org/show_bug.cgi?id=141106.
After this patch, we'll extend it with a new flag, IncludeSymbols.

[Yusuke Suzuki]

Created attachment 249860 [details]
Patch

[Geoffrey Garen]

Comment on attachment 249860 [details]
Patch

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

> Source/JavaScriptCore/runtime/EnumerationMode.h:40
> +class EnumerationMode {
> +public:
> +    static const unsigned s_flagIncludeDontEnumProperties = 1u << 0;
> +    static const unsigned s_flagSkipJSObject = 1u << 1;
> +
> +    enum EnumerationModeRepresentationTag : unsigned {
> +        ExcludeDontEnumProperties = 0,
> +        IncludeDontEnumProperties = s_flagIncludeDontEnumProperties,
> +    };
> +

Since our goal is cleanup, I should say that I don't really love this new design. Mixing an enumeration and a class and a set of flags feels haphazard.

I guess the goal here is to encapsulate enumeration mode into a class in order to gain better control over defaults, and to switch from enumerated values to flags because otherwise adding ExcludeSymbols would produce combinatorial explosion. I think I support those goals. I would do that like so:

enum class DontEnumPropertiesMode {
    Include,
    Exclude
}

enum class JSObjectPropertiesMode {
    Include,
    Exclude
}

enum class SymbolPropertiesMode {
    Include,
    Exclude
}

class EnumerationMode {
public:
    EnumerationMode(DontEnumPropertiesMode = Exclude, JSObjectPropertiesMode = Include, SymbolPropertiesMode = Exclude) { ... }
    EnumerationMode(const EnumerationMode&, JSObjectPropertiesMode) // Convenience for creating a derivative mode to skip JSObject properties
    // Add other constructors as needed for convenience
    
    bool includeDontEnumProperties() { return m_dontEnumPropertiesMode == Include; }
    bool includeJSObjectProperties() { return m_jsObjectPropertiesMode == Include; }
    bool includeSymbolProperties() { return symbolPropertiesMode == Include; }

private:
    DontEnumPropertiesMode m_dontEnumPropertiesMode;
    JSObjectPropertiesMode m_jsObjectPropertiesMode;
    SymbolPropertiesMode m_ symbolPropertiesMode;
};

I think this structure is clearer than having a class, an enum, and a set of flags.

I think it's OK that this structure is slightly larger because it is always stored on the stack. If benchmarks prove that we need to, we can pack our three enums into an unsigned using shifting and masking, and still keep the rest of this interface the same.

[Yusuke Suzuki]

Created attachment 249915 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 249860 [details]
Patch

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

>> Source/JavaScriptCore/runtime/EnumerationMode.h:40
>> +
> 
> Since our goal is cleanup, I should say that I don't really love this new design. Mixing an enumeration and a class and a set of flags feels haphazard.
> 
> I guess the goal here is to encapsulate enumeration mode into a class in order to gain better control over defaults, and to switch from enumerated values to flags because otherwise adding ExcludeSymbols would produce combinatorial explosion. I think I support those goals. I would do that like so:
> 
> enum class DontEnumPropertiesMode {
>     Include,
>     Exclude
> }
> 
> enum class JSObjectPropertiesMode {
>     Include,
>     Exclude
> }
> 
> enum class SymbolPropertiesMode {
>     Include,
>     Exclude
> }
> 
> class EnumerationMode {
> public:
>     EnumerationMode(DontEnumPropertiesMode = Exclude, JSObjectPropertiesMode = Include, SymbolPropertiesMode = Exclude) { ... }
>     EnumerationMode(const EnumerationMode&, JSObjectPropertiesMode) // Convenience for creating a derivative mode to skip JSObject properties
>     // Add other constructors as needed for convenience
>     
>     bool includeDontEnumProperties() { return m_dontEnumPropertiesMode == Include; }
>     bool includeJSObjectProperties() { return m_jsObjectPropertiesMode == Include; }
>     bool includeSymbolProperties() { return symbolPropertiesMode == Include; }
> 
> private:
>     DontEnumPropertiesMode m_dontEnumPropertiesMode;
>     JSObjectPropertiesMode m_jsObjectPropertiesMode;
>     SymbolPropertiesMode m_ symbolPropertiesMode;
> };
> 
> I think this structure is clearer than having a class, an enum, and a set of flags.
> 
> I think it's OK that this structure is slightly larger because it is always stored on the stack. If benchmarks prove that we need to, we can pack our three enums into an unsigned using shifting and masking, and still keep the rest of this interface the same.

Looks nice. OK, so, I'll use the above style EnumerationMode and use EnumerationMode() when the default mode is required.

[Geoffrey Garen]

Comment on attachment 249915 [details]
Patch

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

r=me, but this doesn't build correctly yet.

> Source/JavaScriptCore/bindings/ScriptValue.cpp:140
> +        object->methodTable()->getOwnPropertyNames(object, scriptState, propertyNames, EnuemrationMode());

Maybe getOwnPropertyNames should make EnumerationMode() the default, so we don't have to specify it in all these places.

[Yusuke Suzuki]

Comment on attachment 249915 [details]
Patch

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

>> Source/JavaScriptCore/bindings/ScriptValue.cpp:140
>> +        object->methodTable()->getOwnPropertyNames(object, scriptState, propertyNames, EnuemrationMode());
> 
> Maybe getOwnPropertyNames should make EnumerationMode() the default, so we don't have to specify it in all these places.

Right. So I'll drop the EnumerationMode() :)

[Yusuke Suzuki]

Comment on attachment 249915 [details]
Patch

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

>>> Source/JavaScriptCore/bindings/ScriptValue.cpp:140
>>> +        object->methodTable()->getOwnPropertyNames(object, scriptState, propertyNames, EnuemrationMode());
>> 
>> Maybe getOwnPropertyNames should make EnumerationMode() the default, so we don't have to specify it in all these places.
> 
> Right. So I'll drop the EnumerationMode() :)

Oops. Since this `methodTable()->getOwnPropertyNames` is function pointer, we cannot call it with default parameter.
I'll fix the build error.

[Yusuke Suzuki]

Created attachment 249958 [details]
Patch

[Geoffrey Garen]

Comment on attachment 249958 [details]
Patch

r=me

[Yusuke Suzuki]

Comment on attachment 249958 [details]
Patch

Thank you for your review and r+! Yay, so it's time to update Object.getOwnPropertySymbols patch :D

[WebKit Commit Bot]

Comment on attachment 249958 [details]
Patch

Clearing flags on attachment: 249958

Committed r182280: <http://trac.webkit.org/changeset/182280>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.