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

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied 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 132649: Patch
https://bugs.webkit.org/attachment.cgi?id=132649&action=review

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


Looking better but r- for various style and other issues.

> Source/WebCore/css/CSSParser.cpp:484
> +static inline bool propertyMatchKeyword(int propertyId, int id)
> +{

The name here is not great. Maybe isValidKeywordPropertyAndValue() or similar?

What 'id'? It should be called 'valueID'. I think we generally capitalize ID so
'propertyID'.

> Source/WebCore/css/CSSParser.cpp:489
> +    case CSSPropertyBorderCollapse: // collapse | separate | inherit
> +	       if (id == CSSValueCollapse || id == CSSValueSeparate)
> +		   return true;
> +	       break;

Double indentation in this switch

> Source/WebCore/css/CSSParser.cpp:899
> +    CSSParserString cssString;
> +    cssString.characters = const_cast<UChar*>(string.characters());
> +    cssString.length = string.length();
> +    int valueID = cssValueKeywordID(cssString);

This could be an inline function. Surprised that there is not one already.

> Source/WebCore/css/CSSParser.cpp:909
> +	   if (cssValuePool)
> +	   value = cssValuePool ? cssValuePool->createInheritedValue() :
CSSInheritedValue::create();

did you forget that if() there?

> Source/WebCore/css/CSSParser.cpp:924
> +	   declaration->addParsedProperty(CSSProperty(propertyId,
value.release(), important));
> +	   return true;
> +    }
> +    if (valueID == CSSValueInitial) {
> +	   value = cssValuePool ? cssValuePool->createExplicitInitialValue() :
CSSInitialValue::createExplicit();
> +	   declaration->addParsedProperty(CSSProperty(propertyId,
value.release(), important));
> +	   return true;
> +    }
> +
> +    if (!propertyMatchKeyword(propertyId, valueID))
> +	   return false;
> +
> +    value = document ? cssValuePool->createIdentifierValue(valueID) :
CSSPrimitiveValue::createIdentifier(valueID);
> +    declaration->addParsedProperty(CSSProperty(propertyId, value.release(),
important));
> +    return true;

The code might look nicer and be shorter if factored to create CSSValues in
branches and then do declaration->addParsedProperty() once at the end.
Basically not using early returns for inherit and initial.

> Source/WebCore/css/CSSParser.cpp:1381
>      bool validPrimitive = false;
>      RefPtr<CSSValue> parsedValue;
>  
> +    if (id && isKeywordPropertyID(propId)) {
> +	       if (!propertyMatchKeyword(propId, id))

You don't need to invoke isKeywordPropertyID() here at all,
propertyMatchKeyword() returns false for non-keyword properties.

if() here should be before the initialization of those variables. They are not
needed if the branch is taken.

Indentation.

> Source/WebCore/css/CSSParser.cpp:1383
> +		   return false;
> +	       parsedValue = parseValidPrimitive(id, value);

Why does this call to parseValidPrimitive? You know it is an identifier value,
you can just do cssValuePool()->createIdentifierValue(id).

> Source/WebCore/css/CSSParser.cpp:1390
> +	       if (!m_valueList->current() || inShorthand()) {
> +		   addProperty(propId, parsedValue.release(), important);
> +		   return true;
> +	       }
> +	       return false;

Would be nicer to switch these branches around as you won't need the block at
all.