Re: using enum classes for enumerated CSS property values in nsStyleConsts.h

L. David Baron:
> 1. Naming
> =========
> 
> Do the names I chose seem reasonable?  I replaced:
>   -#define NS_STYLE_BOX_SIZING_CONTENT       0
>   -#define NS_STYLE_BOX_SIZING_PADDING       1
>   -#define NS_STYLE_BOX_SIZING_BORDER        2
> with:
>   +namespace mozilla {
>   +
>   +enum class StyleBoxSizing : uint8_t {
>   +  Content,
>   +  Padding,
>   +  Border
>   +};
>   +
>   +} // namespace mozilla
> so that most caller changes look like:
>   -    case NS_STYLE_BOX_SIZING_BORDER:
>   +    case StyleBoxSizing::Border:
> or like:
>      if (parent &&
>   -      parent->StylePosition()->mBoxSizing != NS_STYLE_BOX_SIZING_BORDER) {
>   +      parent->StylePosition()->mBoxSizing != StyleBoxSizing::Border) {

Those names look good to me.

> 2. Casts
> ========
> 
> Does anybody see a good way to eliminate the casts I needed to use
> in the main patch (described in the patch header)?

For the KTables, you could define KTableValue as a class that has
implicit conversion constructors taking nsCSSKeyword and StyleBoxSizing
(and all the other new ones when they’re added).

For the casts in SetDiscrete, no, although I don’t think it’s much of a
problem.  (I suppose you could do something where you have a template
wrapper class for nsCSSValue that identifies the particular enum that an
eCSSUnit_Enumerated in the wrapped nsCSSValue represents, and then have
nsRuleData::ValueForXXX return types like
nsCSSValueWrapper<StyleBoxSizing>, but it seems over the top.)

> 3. General
> ==========
> 
> Do others agree this is a good idea?  And are people ok with it
> being done gradually?

Yes, I think it’s great.

-- 
Cameron McCormack ≝ http://mcc.id.au/
_______________________________________________
dev-tech-layout mailing list
[email protected]
https://lists.mozilla.org/listinfo/dev-tech-layout