Bug 143276

Created attachment 249860 [details] Patch

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.

Created attachment 249915 [details] Patch

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.

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.

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() :)

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.

Created attachment 249958 [details] Patch

Comment on attachment 249958 [details] Patch r=me

Comment on attachment 249958 [details] Patch Thank you for your review and r+! Yay, so it's time to update Object.getOwnPropertySymbols patch :D

Comment on attachment 249958 [details] Patch Clearing flags on attachment: 249958 Committed r182280: <http://trac.webkit.org/changeset/182280>

All reviewed patches have been landed. Closing bug.