Bug 65662 – Implement string based properties in CSSStyleApplyProperty.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Luke Macpherson]

Implement string based properties in CSSStyleApplyProperty.

[Luke Macpherson]

Created attachment 102871 [details]
Patch

[Darin Adler]

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.

[Luke Macpherson]

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.

[Luke Macpherson]

Created attachment 103427 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 103427 [details]
Patch for landing

Clearing flags on attachment: 103427

Committed r92742: <http://trac.webkit.org/changeset/92742>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.