[webkit-reviews] review denied: [Bug 51586] CSSParser: m_implicitShorthand should probably be RAII : [Attachment 94630] [PATCH] Enum instead of bool

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 51586: CSSParser: m_implicitShorthand should probably be RAII
https://bugs.webkit.org/show_bug.cgi?id=51586

Attachment 94630: [PATCH] Enum instead of bool
https://bugs.webkit.org/attachment.cgi?id=94630&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=94630&action=review

> Source/WebCore/css/CSSParser.cpp:100
> +    enum PropertyType {

Making this enum a class member makes the call sites ugly with extra
qualification needed. How about just making this a namespace-level enum
instead?

> Source/WebCore/css/CSSParser.cpp:101
> +	   PropertyExplicit = 0,

Do we really need the "= 0" here? Please omit it.

> Source/WebCore/css/CSSParser.cpp:105
> +    ImplicitScope(WebCore::CSSParser* parser, PropertyType propertyType)

I don’t think we need an argument at all. This class is already named
ImplicitScope. I suggest removing the argument completely and just having this
set to true.

> Source/WebCore/css/CSSParser.cpp:108
> +	   m_parser->m_implicitShorthand = static_cast<bool>(propertyType);

I don’t think the static_cast here is good style. Is it required by some
compiler? Actually, I would suggest writing this:

    m_parser->m_implicitShorthand = propertyType == PropertyImplicit;

I think that’s clearer.