[webkit-reviews] review granted: [Bug 76229] Remove external references to CSSPrimitiveValue::UnitTypes enum. : [Attachment 122363] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 76229: Remove external references to CSSPrimitiveValue::UnitTypes enum.
https://bugs.webkit.org/show_bug.cgi?id=76229

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

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


Patch seems good as is. Other flaws in the code might be worth fixing in a
follow-up.

> Source/WebCore/css/CSSFontSelector.cpp:290
> +	   else if (item->isIdent()) {

Seems like we could just leave this check out. The code below quickly does
nothing if it’s not an ident. Creates a null string, does a switch statement,
then bails out because the family name is empty.

> Source/WebCore/css/CSSFontSelector.cpp:293
>	       String familyName;

Not new to your patch, but: This extra declaration of familyName prevents the
following switch statement from doing anything at all. This code doesn’t work!

> Source/WebCore/css/CSSPrimitiveValue.h:119
> +    static bool isUnitTypeLength(int type) { return (type > CSS_PERCENTAGE
&& type < CSS_DEG) || type == CSS_REMS; }

Seems like this could be private if we aren’t going to expose types outside the
class.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:1029
> -		   } else if (type == CSSPrimitiveValue::CSS_IDENT)
> +		   } else if (primitiveValue->isIdent())
>		       selector->style()->setCursor(*primitiveValue);

I wonder if this code really needs to work this way. If the value is not an
identifier, then it carefully does nothing. But if it’s an identifier there is
nothing here that checks to see that it’s one of the identifiers that specifies
a cursor. I wonder if the isIdent check really is needed.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:1234
>		   switch (primitiveValue->getIdent()) {
> +		   case 0:
> +		       return;

Seems strange that if there is an identifier we are guaranteed it’s a valid
one, but that it’s possible the primitive value isn’t a value one at all. I
suspect this 0-ident code path isn’t actually reached.