[webkit-reviews] review denied: [Bug 57964] Fast path for parsing simple CSS values : [Attachment 88518] updated patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied 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 88518: updated patch
https://bugs.webkit.org/attachment.cgi?id=88518&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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.