[webkit-reviews] review granted: [Bug 67139] Early return in CSSPrimitiveValue::getDoubleValueInternal() omits additional invalid enums : [Attachment 105643] [PATCH] Suggested fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 67139: Early return in CSSPrimitiveValue::getDoubleValueInternal() omits
additional invalid enums
https://bugs.webkit.org/show_bug.cgi?id=67139

Attachment 105643: [PATCH] Suggested fix
https://bugs.webkit.org/attachment.cgi?id=105643&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105643&action=review


r=me even though I think we can make some small improvements

> Source/WebCore/css/CSSPrimitiveValue.cpp:53
> +    switch (unitType) {

Using a switch is a great technique to make sure we make a conscious decision
for each enum value. But I’d suggest just having one block for false and one
for true, sorted alphabetically. The order now may have some logic to it, but
it’s not obvious and I think it would be better to not have that.

> Source/WebCore/css/CSSPrimitiveValue.cpp:86
> +    case CSSPrimitiveValue:: CSS_UNICODE_RANGE:
> +
> +    case CSSPrimitiveValue:: CSS_PARSER_OPERATOR:

This blank line looks like a mistake.