[webkit-reviews] review denied: [Bug 51586] CSSParser: m_implicitShorthand should probably be RAII : [Attachment 93633] [PATCH] Comments addressed

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> 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 93633: [PATCH] Comments addressed
https://bugs.webkit.org/attachment.cgi?id=93633&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=93633&action=review

> Source/WebCore/css/CSSParser.cpp:98
> +    WTF_MAKE_FAST_ALLOCATED;

Why are you adding this here?

What about non-copyable?

> Source/WebCore/css/CSSParser.cpp:100
> +    ImplicitScope(WebCore::CSSParser* parser, bool isImplicit) :
m_parser(parser)

Put initialization for m_parser on a new line.

Why pass in isImplicit when it is always true?

> Source/WebCore/css/CSSParser.cpp:102
> +	  m_parser->m_implicitShorthand = isImplicit;

It looks like you have a 3 space indent instead of 4 spaces.

> Source/WebCore/css/CSSParser.cpp:103
> +    }

add blank line here.