Bug 75104 - CSSParser: Avoid creating dummy declaration in parseColor() slow path.
Summary: CSSParser: Avoid creating dummy declaration in parseColor() slow path.
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
Depends on:
Reported: 2011-12-22 11:41 PST by Andreas Kling
Modified: 2011-12-23 14:34 PST (History)
2 users (show)

See Also:

Proposed patch (5.86 KB, patch)
2011-12-22 11:44 PST, Andreas Kling
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2011-12-22 11:41:29 PST
The slow-path in CSSParser::parseColor() currently creates a CSSMutableStyleDeclaration in order to trick the CSSParser into instantiating a local CSSValuePool. We shouldn't have to do that.
Comment 1 Andreas Kling 2011-12-22 11:44:12 PST
Created attachment 120354 [details]
Proposed patch
Comment 2 WebKit Review Bot 2011-12-22 14:19:02 PST
Attachment 120354 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/css/CSSParser.h:154:  The parameter name "rgb" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 3 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2011-12-22 14:46:03 PST
Comment on attachment 120354 [details]
Proposed patch

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.
Comment 4 Andreas Kling 2011-12-23 14:34:11 PST
Committed r103639: <http://trac.webkit.org/changeset/103639>