[webkit-reviews] review granted: [Bug 75104] CSSParser: Avoid creating dummy declaration in parseColor() slow path. : [Attachment 120354] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <kling at webkit.org>'s
request for review:
Bug 75104: CSSParser: Avoid creating dummy declaration in parseColor() slow
path.
https://bugs.webkit.org/show_bug.cgi?id=75104

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

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


> Source/WebCore/css/CSSParser.cpp:520
> +    ensureCSSValuePool();

Since it’s so non-obvious that this call is needed, I think we should add a
comment to explain why it is.

> Source/WebCore/css/CSSParser.cpp:663
> +void CSSParser::ensureCSSValuePool()
> +{
> +    if (!m_cssValuePool)
> +	   m_cssValuePool = CSSValuePool::create();
> +}

Seems a little strange to add a function like this that is only called in one
place. I probably would have put it at the top of the source file and marked it
inline.

>> Source/WebCore/css/CSSParser.h:154
>> +	static bool fastParseColor(const String&, RGBA32& rgb, bool strict);
> 
> The parameter name "rgb" adds no information, so it should be removed. 
[readability/parameter_name] [5]

I agree with Mr. Style Queue. We should either use no name or the name color.

I also probably would have put the out parameter either last or first, rather
than between two other arguments. It seems crazy, but this used to be
distinguished from the real full parseColor only because it had arguments in a
different order. We should put the arguments in the same order.