Fast path for parsing simple CSS values

[Ian Henderson]

Javascript animations repeatedly set style properties of an element to values like "10px" or "5%".  We should optimize this common case.

[Ian Henderson]

Created attachment 88473 [details]
proposed patch

[WebKit Review Bot]

Attachment 88473 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/css/CSSParser.cpp:303:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSParser.cpp:377:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 88473 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88473&action=review

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

Make inline?

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

Say Length instead of DimensionValue?

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

Please rename 'id' to 'propId' or 'propertyId'.

[Dave Hyatt]

Comment on attachment 88473 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88473&action=review

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

It's not clear to me why we need to call this "isSimpleColorPropertyID"... as opposed to what... a complex color property ID?  It also seems like a bunch of color properties are missing from this list. What about outline-color and all of the various SVG color properties?

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

Are there colors you're omitting on purpose? Still not clear on the terminology unless maybe that's the case....

> Source/WebCore/css/CSSParser.cpp:322
> +static bool isSimpleDimensionPropertyID(int id, bool& acceptsNegativeNumbers)

What about the padding properties?  Also, as SImon points out, I don't think "dimension" is a good term here, since that has meaning in CSS.  I would use Length.

[Antti Koivisto]

Comment on attachment 88473 [details]
proposed patch

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?

[Antti Koivisto]

Comment on attachment 88473 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88473&action=review

> Source/WebCore/css/CSSParser.cpp:308
> +        CSSProperty property(id, CSSPrimitiveValue::createIdentifier(valueID), important);

You should construct CSSPrimitiveValue using CSSPrimitiveValueCache. That hangs from the document which you can get through the style declaration.

> Source/WebCore/css/CSSParser.cpp:316
> +    CSSProperty property(id, CSSPrimitiveValue::createColor(color), important);

You should construct CSSPrimitiveValue using CSSPrimitiveValueCache. That hangs from the document which you can get through the style declaration.

> Source/WebCore/css/CSSParser.cpp:384
> +    CSSProperty property(id, CSSPrimitiveValue::create(number, unit), important);

You should construct CSSPrimitiveValue using CSSPrimitiveValueCache. That hangs from the document which you can get through the style declaration.

[Ian Henderson]

Created attachment 88518 [details]
updated patch

[Simon Fraser (smfr)]

Comment on attachment 88518 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88518&action=review

r- for the "px" parsing issue.

> Source/WebCore/css/CSSParser.cpp:281
> +    case CSSPropertyBackgroundColor:
> +    case CSSPropertyBorderTopColor:
> +    case CSSPropertyBorderRightColor:
> +    case CSSPropertyBorderBottomColor:
> +    case CSSPropertyBorderLeftColor:
> +    case CSSPropertyWebkitBorderStartColor:
> +    case CSSPropertyWebkitBorderEndColor:
> +    case CSSPropertyWebkitBorderBeforeColor:
> +    case CSSPropertyWebkitBorderAfterColor:
> +    case CSSPropertyColor:
> +    case CSSPropertyTextLineThroughColor:
> +    case CSSPropertyTextUnderlineColor:
> +    case CSSPropertyTextOverlineColor:
> +    case CSSPropertyWebkitColumnRuleColor:
> +    case CSSPropertyWebkitTextEmphasisColor:
> +    case CSSPropertyWebkitTextFillColor:
> +    case CSSPropertyWebkitTextStrokeColor:

These might be better ordered by ID, rather than alphabetically, for potential compiler optimization, and easy of matching with the enum of all properties.

What about CSSPropertyOutlineColor, CSSPropertyWebkitColumnRuleColor?

> Source/WebCore/css/CSSParser.cpp:295
> +    RefPtr<CSSPrimitiveValueCache> primitiveValueCache = stylesheet->document()->cssPrimitiveValueCache();

I don't think you need a RefPtr to the cssPrimitiveValueCache. You could also fetch it later.

> Source/WebCore/css/CSSParser.cpp:297
> +    if (!string.length())
> +        return false;

Why not check this earlier?

> Source/WebCore/css/CSSParser.cpp:374
> +    RefPtr<CSSPrimitiveValueCache> primitiveValueCache = stylesheet->document()->cssPrimitiveValueCache();

Ditto. Also, why not fetch this just before you use it lower down?

> Source/WebCore/css/CSSParser.cpp:384
> +    if (length > 2 && characters[length - 2] == 'p' && characters[length - 1] == 'x') {
> +        length -= 2;
> +        unit = CSSPrimitiveValue::CSS_PX;

This would break if we ever created a unit like "ppx" or 'screenpx". I think you need to walk back looking for non-unit characters.

> Source/WebCore/css/CSSParser.cpp:387
> +    } else if (length > 1 && characters[length - 1] == '%') {
> +        length -= 1;
> +        unit = CSSPrimitiveValue::CSS_PERCENTAGE;

Ditto if a "%%" unit was invented.

[Ian Henderson]

Created attachment 88534 [details]
updated patch 2

Updated to address Simon's comments.  The "px" issue doesn't actually happen, since charactersToDouble validates its input.  I added a comment to emphasize this.

[WebKit Review Bot]

Attachment 88534 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/css/CSSParser.cpp:386:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/83122