[Webkit-unassigned] [Bug 57964] Fast path for parsing simple CSS values

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=57964


Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #88518|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #8 from Simon Fraser (smfr) <simon.fraser at apple.com>  2011-04-06 14:58:38 PST ---
(From update of attachment 88518)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.