[webkit-reviews] review granted: [Bug 71668] CSSValue: Devirtualize isFooType(). : [Attachment 113904] Proposed patch v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <kling at webkit.org>'s
request for review:
Bug 71668: CSSValue: Devirtualize isFooType().
https://bugs.webkit.org/show_bug.cgi?id=71668

Attachment 113904: Proposed patch v2
https://bugs.webkit.org/attachment.cgi?id=113904&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.