Bug 76229 - Remove external references to CSSPrimitiveValue::UnitTypes enum.
Summary: Remove external references to CSSPrimitiveValue::UnitTypes enum.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-12 16:57 PST by Luke Macpherson
Modified: 2012-01-15 15:18 PST (History)
2 users (show)

See Also:


Attachments
Patch (15.85 KB, patch)
2012-01-12 17:03 PST, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (16.80 KB, patch)
2012-01-12 20:28 PST, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2012-01-12 16:57:53 PST
Remove external references to CSSPrimitiveValue::UnitTypes enum.
Comment 1 Luke Macpherson 2012-01-12 17:03:47 PST
Created attachment 122341 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-12 18:22:57 PST
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
Comment 3 Luke Macpherson 2012-01-12 20:28:02 PST
Created attachment 122363 [details]
Patch
Comment 4 Darin Adler 2012-01-12 23:16:35 PST
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.
Comment 5 Luke Macpherson 2012-01-15 13:47:49 PST
Thank Darin, I'll post a follow-up patch with those suggestions once this lands,
Comment 6 WebKit Review Bot 2012-01-15 15:18:45 PST
Comment on attachment 122363 [details]
Patch

Clearing flags on attachment: 122363

Committed r105033: <http://trac.webkit.org/changeset/105033>
Comment 7 WebKit Review Bot 2012-01-15 15:18:50 PST
All reviewed patches have been landed.  Closing bug.