Implement string based properties in CSSStyleApplyProperty.
Created attachment 102871 [details] Patch
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 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.
Created attachment 103427 [details] Patch for landing
Comment on attachment 103427 [details] Patch for landing Clearing flags on attachment: 103427 Committed r92742: <http://trac.webkit.org/changeset/92742>
All reviewed patches have been landed. Closing bug.