Bug 76229 – Remove external references to CSSPrimitiveValue::UnitTypes enum.

• Component: New Bugs
• Status: RESOLVED FIXED

[Luke Macpherson]

Remove external references to CSSPrimitiveValue::UnitTypes enum.

[Luke Macpherson]

Created attachment 122341 [details]
Patch

[WebKit Review Bot]

Comment on attachment 122341 [details]
Patch

Attachment 122341 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11157346

New failing tests:
fast/gradients/css3-radial-gradients2.html
printing/page-format-data.html
fast/gradients/css3-radial-gradients.html

[Luke Macpherson]

Created attachment 122363 [details]
Patch

[Darin Adler]

Comment on attachment 122363 [details]
Patch

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.

[Luke Macpherson]

Thank Darin, I'll post a follow-up patch with those suggestions once this lands,

[WebKit Review Bot]

Comment on attachment 122363 [details]
Patch

Clearing flags on attachment: 122363

Committed r105033: <http://trac.webkit.org/changeset/105033>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.