[webkit-reviews] review denied: [Bug 117481] Make sure to use CSSValueID and CSSPropertyID rather than integers : [Attachment 205020] attempt to fix efl build

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Alexis Menard (darktears)
<alexis at webkit.org>'s request for review:
Bug 117481: Make sure to use CSSValueID and CSSPropertyID rather than integers
https://bugs.webkit.org/show_bug.cgi?id=117481

Attachment 205020: attempt to fix efl build
https://bugs.webkit.org/attachment.cgi?id=205020&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=205020&action=review


> Source/WebCore/css/CSSParser.cpp:5646
> +	       addProperty(CSSPropertyFontWeight,
cssValuePool().createIdentifierValue(static_cast<CSSValueID>(CSSValue100 +
weight / 100 - 1)), important);

I reeeeaaally don't like this.

I would make it a separate function, with pre and post assertions, and a
comment.

> Source/WebCore/css/CSSParser.cpp:12236
> -	       return 0; // illegal character
> +	       return CSSValueInvalid; // illegal character

Fix the comment.

> Source/WebCore/css/CSSValuePool.cpp:55
> -    if (ident <= 0 || ident >= numCSSValueKeywords)
> +    if (ident <= 0)
>	   return CSSPrimitiveValue::createIdentifier(ident);

1) Why would that happen with valid CSSValueID? Shouldn't you instead have two
constructors?: one taking an int, the other taking a CSSValueID.

> Source/WebCore/css/CSSValuePool.cpp:57
>      if (!m_identifierValueCache[ident])

2) Because of this, you should have a Release Assert for the range of ident!