[webkit-reviews] review denied: [Bug 57964] Fast path for parsing simple CSS values : [Attachment 88518] updated patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Apr 6 14:58:38 PDT 2011
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.
More information about the webkit-reviews
mailing list