[webkit-reviews] review granted: [Bug 80874] Implement a fast path when setting CSS properties with keywords from JS. : [Attachment 132693] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Alexis Menard (darktears)
<alexis.menard at openbossa.org>'s request for review:
Bug 80874: Implement a fast path when setting CSS properties with keywords from
JS.
https://bugs.webkit.org/show_bug.cgi?id=80874

Attachment 132693: Patch
https://bugs.webkit.org/attachment.cgi?id=132693&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=132693&action=review


r=me, with a few minor comments

> Source/WebCore/css/CSSParser.cpp:792
> +    case CSSPropertyWordWrap: // normal | break-word
> +	   if (valueID == CSSValueNormal || valueID == CSSValueBreakWord)
> +	       return true;
> +	   break;
> +    default:
> +	   return false;
> +    }

You should ASSERT_NOT_REACHED() in default case.

> Source/WebCore/css/CSSParser.cpp:919
> +    if (valueID == CSSValueInherit)
> +	   value = cssValuePool ? cssValuePool->createInheritedValue() :
CSSInheritedValue::create();
> +    if (valueID == CSSValueInitial)
> +	   value = cssValuePool ? cssValuePool->createExplicitInitialValue() :
CSSInitialValue::createExplicit();
> +
> +    if (!value && !isValidKeywordPropertyAndValue(propertyId, valueID))
> +	   return false;
> +
> +    if (!value)
> +	   value = document ? cssValuePool->createIdentifierValue(valueID) :
CSSPrimitiveValue::createIdentifier(valueID);

This can be still improved. Use 'else if's. You won't need the !value tests.

> Source/WebCore/css/CSSParser.cpp:1374
> +    if (id && isKeywordPropertyID(propId)) {

Can the id really be 0? ASSERT(id) perhaps?