[webkit-reviews] review denied: [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 denied  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


Switched to r- for concerns over invalid value handling and breaking the no-svg
build.

>> Source/WebCore/css/CSSParser.cpp:1374
>> +	if (id && isKeywordPropertyID(propId)) {
> 
> Can the id really be 0? ASSERT(id) perhaps?

Actually the id test is wrong. You will end up in the main switch which won't
know how to handle the propId. If id==CSSValueInvalid can actually happen then
it needs to be handled within isKeywordPropertyID() branch maybe by having
isValidKeywordPropertyAndValue() test it.

> Source/WebCore/css/CSSParser.cpp:-979
> -    case CSSPropertyPosition:	     // static | relative | absolute |
fixed | inherit
> -	   if (id == CSSValueStatic

In non-SVG build the switch won't have default: case so it won't build after
your change.

I think you should keep these properties in the main switch, together without
implementation, and ASSERT_NOT_REACHED.