[webkit-reviews] review granted: [Bug 57964] Fast path for parsing simple CSS values : [Attachment 88473] proposed patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Ian Henderson <ianh at apple.com>'s
request for review:
Bug 57964: Fast path for parsing simple CSS values
https://bugs.webkit.org/show_bug.cgi?id=57964

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

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=88473&action=review

Looks good, r=me with some comments

> Source/WebCore/css/CSSParser.cpp:262
> +static bool isSimpleColorPropertyID(int id)

This could be inline.
id is too generic, call it propertyID.

> Source/WebCore/css/CSSParser.cpp:288
> +static bool parseSimpleColorValue(CSSMutableStyleDeclaration* declaration,
int id, const String& string, bool important, bool strict)

id -> propertyID

> Source/WebCore/css/CSSParser.cpp:318
> +    if (validPrimitive) {
> +	   CSSProperty property(id,
CSSPrimitiveValue::createIdentifier(valueID), important);
> +	   CSSProperty* propertyArray = &property;
> +	   declaration->addParsedProperties(&propertyArray, 1);
> +	   return true;
> +    }
> +    RGBA32 color;
> +    if (!CSSParser::parseColor(string, color, strict && string[0] != '#'))
> +	   return false;
> +    CSSProperty property(id, CSSPrimitiveValue::createColor(color),
important);
> +    CSSProperty* propertyArray = &property;
> +    declaration->addParsedProperties(&propertyArray, 1);

There is CSSMutableStyleDeclaration::addParsedProperty() for adding a single
property. Perhaps you can use that?

> Source/WebCore/css/CSSParser.cpp:331
> +    case CSSPropertyWebkitLogicalHeight:
> +	   acceptsNegativeNumbers = false;
> +	   return true;
> +

remove empty line

> Source/WebCore/css/CSSParser.cpp:346
> +    case CSSPropertyWebkitMarginAfter:
> +	   acceptsNegativeNumbers = true;
> +	   return true;
> +

remove empty line

> Source/WebCore/css/CSSParser.cpp:353
> +static bool parseSimpleDimensionValue(CSSMutableStyleDeclaration*
declaration, int id, const String& string, bool important, bool strict)
> +{

id -> propertyID

> Source/WebCore/css/CSSParser.cpp:386
> +    CSSProperty property(id, CSSPrimitiveValue::create(number, unit),
important);
> +    CSSProperty* propertyArray = &property;
> +    declaration->addParsedProperties(&propertyArray, 1);

There is CSSMutableStyleDeclaration::addParsedProperty() for adding a single
property. Perhaps you can use that?