CSSValue: Devirtualize isFooType().
Created attachment 113847 [details] Patch (depends on 71667)
Comment on attachment 113847 [details] Patch (depends on 71667) View in context: https://bugs.webkit.org/attachment.cgi?id=113847&action=review > Source/WebCore/css/CSSPrimitiveValueMappings.h:1342 > + : CSSValue(PrimitiveClass, CSS_PRIMITIVE_VALUE, /* isPrimitive */ true) Kinda hatin' here...
(In reply to comment #2) > (From update of attachment 113847 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113847&action=review > > > Source/WebCore/css/CSSPrimitiveValueMappings.h:1342 > > + : CSSValue(PrimitiveClass, CSS_PRIMITIVE_VALUE, /* isPrimitive */ true) > > Kinda hatin' here... Reviewers gonna review. I'll redesign as discussed.
Created attachment 113904 [details] Proposed patch v2 A little less verbose this time. ;)
Comment on attachment 113904 [details] Proposed patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=113904&action=review > Source/WebCore/css/CSSImageValue.h:53 > + CSSImageValue(const String& url); This single-argument constructor should probably be marked explicit. > Source/WebCore/css/CSSMutableValue.h:43 > + CSSMutableValue(ClassType classType) This single-argument constructor should probably be marked explicit. > Source/WebCore/css/CSSPrimitiveValue.h:225 > + CSSPrimitiveValue(int ident); > CSSPrimitiveValue(unsigned color); // RGB value > CSSPrimitiveValue(const Length&); We should explore putting the explicit keyword on these single-argument constructors. > Source/WebCore/css/CSSValue.h:135 > + CSSValue(ClassType classType) > + : m_classType(classType) > + , m_isPrimitive(isPrimitiveType(classType)) > + , m_isMutable(isMutableType(classType)) > + , m_isList(isListType(classType)) > + , m_isInitial(isInitialType(classType)) > + , m_isInherited(isInheritedType(classType)) Another way to do this is to include bits to indicate these properties in the constants that are passed in. > Source/WebCore/css/CSSValue.h:145 > + || type == ImageClass > + || type == CursorImageClass > + || type == FontFamilyClass; These are indented 7 spaces. Instead should be indented 4 spaces. > Source/WebCore/css/CSSValueList.h:68 > + CSSValueList(bool isSpaceSeparated); > + CSSValueList(CSSParserValueList*); Should use explicit here. > Source/WebCore/svg/SVGColor.h:83 > + SVGColor(const SVGColorType&); Should mark this explicit.
Committed r99468: <http://trac.webkit.org/changeset/99468>
Hi Andreas, what was the rationale for caching m_isPrimitiveValue and m_isList rather than just calculating from m_classType when needed? Could we just rearrange the ClassType Enum to allow the inline check to be a single comparison?
(In reply to comment #7) > Hi Andreas, what was the rationale for caching m_isPrimitiveValue and m_isList rather than just calculating from m_classType when needed? > > Could we just rearrange the ClassType Enum to allow the inline check to be a single comparison? Hi Luke, The idea was to keep isPrimitiveValue() and isValueList() as cheap as possible as a "speculative optimization" since they are used quite a bit. If you have an idea for removing the need for them without incurring additional compares, I'm all ears :)
*** Bug 64863 has been marked as a duplicate of this bug. ***