Bug 65662 - Implement string based properties in CSSStyleApplyProperty.
Summary: Implement string based properties in CSSStyleApplyProperty.
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: 2011-08-03 20:42 PDT by Luke Macpherson
Modified: 2011-08-09 18:44 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.71 KB, patch)
2011-08-03 20:52 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch for landing (6.80 KB, patch)
2011-08-09 18:18 PDT, 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 2011-08-03 20:42:43 PDT
Implement string based properties in CSSStyleApplyProperty.
Comment 1 Luke Macpherson 2011-08-03 20:52:04 PDT
Created attachment 102871 [details]
Patch
Comment 2 Darin Adler 2011-08-05 11:34:20 PDT
Comment on attachment 102871 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102871&action=review

Looks OK, but could be improved, I think.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:273
> -enum LengthAuto {AutoDisabled = 0, AutoEnabled = 1};
> -enum LengthIntrinsic {IntrinsicDisabled = 0, IntrinsicEnabled = 1};
> -enum LengthMinIntrinsic {MinIntrinsicDisabled = 0, MinIntrinsicEnabled = 1};
> -enum LengthNone {NoneDisabled = 0, NoneEnabled = 1};
> -enum LengthUndefined {UndefinedDisabled = 0, UndefinedEnabled = 1};
> -enum LengthFlexDirection {FlexDirectionDisabled = 0, FlexWidth = 1, FlexHeight};
> +enum LengthAuto {AutoDisabled = 0, AutoEnabled};
> +enum LengthIntrinsic {IntrinsicDisabled = 0, IntrinsicEnabled};
> +enum LengthMinIntrinsic {MinIntrinsicDisabled = 0, MinIntrinsicEnabled};
> +enum LengthNone {NoneDisabled = 0, NoneEnabled};
> +enum LengthUndefined {UndefinedDisabled = 0, UndefinedEnabled};
> +enum LengthFlexDirection {FlexDirectionDisabled = 0, FlexWidth, FlexHeight};

I don’t understand this change. Why are the "= 0" good and the "= 1" bad? What does this have to do with the rest of the patch.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:335
> +enum StringMapIdentToNull {NothingMapsToNull = 0, MapNoneToNull, MapAutoToNull};

Normal WebKit formatting puts space after the { and before the }.

Why the explicit "= 0"?

Why abbreviate the word identifier to ident? I find it easier to read words than word fragments.

The name of this type is hard for me to understand. What is a "string map ident to null"? I think it needs a word like "policy" or "behavior" in the name to make clear what the type is.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:355
> +    virtual void applyValue(CSSStyleSelector* selector, CSSValue* value) const
> +    {
> +        if (!value->isPrimitiveValue())
> +            return;
> +        CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);
> +        if ((mapIdentToNull == MapNoneToNull && primitiveValue->getIdent() == CSSValueNone)
> +            || (mapIdentToNull == MapAutoToNull && primitiveValue->getIdent() == CSSValueAuto))
> +            setValue(selector->style(), nullAtom);
> +        else
> +            setValue(selector->style(), primitiveValue->getStringValue());
> +    }

I’d rather see this function that won’t be inlined defined out of line rather than indented inside the class definition.
Comment 3 Luke Macpherson 2011-08-09 17:51:11 PDT
Comment on attachment 102871 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102871&action=review

>> Source/WebCore/css/CSSStyleApplyProperty.cpp:273
>> +enum LengthFlexDirection {FlexDirectionDisabled = 0, FlexWidth, FlexHeight};
> 
> I don’t understand this change. Why are the "= 0" good and the "= 1" bad? What does this have to do with the rest of the patch.

The idea here is to make explicit the result of cast to booleans. The 1s are not necessary as any non-zero value will do, while the 0s help communicate that the order of the elements, and the result of casting to boolean are important.

This is just a change in passing, not related to this CL other than maintaining consistency with the newly added enums.

>> Source/WebCore/css/CSSStyleApplyProperty.cpp:335

> 
> Normal WebKit formatting puts space after the { and before the }.
> 
> Why the explicit "= 0"?
> 
> Why abbreviate the word identifier to ident? I find it easier to read words than word fragments.
> 
> The name of this type is hard for me to understand. What is a "string map ident to null"? I think it needs a word like "policy" or "behavior" in the name to make clear what the type is.

1) spaces around braces fixed.
2) see rationale above.
3) good idea. done.

> Source/WebCore/css/CSSStyleSelector.cpp:-4783
> -    }

I have reverted changes to the CSSPropertyWebkitLocale property because they conflict with code that has just gone in.
Comment 4 Luke Macpherson 2011-08-09 18:18:37 PDT
Created attachment 103427 [details]
Patch for landing
Comment 5 WebKit Review Bot 2011-08-09 18:44:22 PDT
Comment on attachment 103427 [details]
Patch for landing

Clearing flags on attachment: 103427

Committed r92742: <http://trac.webkit.org/changeset/92742>
Comment 6 WebKit Review Bot 2011-08-09 18:44:26 PDT
All reviewed patches have been landed.  Closing bug.