[webkit-reviews] review granted: [Bug 65662] Implement string based properties in CSSStyleApplyProperty. : [Attachment 102871] 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 65662: Implement string based properties in CSSStyleApplyProperty.
https://bugs.webkit.org/show_bug.cgi?id=65662

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.