User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0 Build ID: 20160606113944 Steps to reproduce: Nothing to reproduce. Actual results: Currently using NS_STYLE_IMAGELAYER_POSITION macro definitions. Expected results: Changed this to Enum Class StyleImageLayerPosition.

Comment on attachment 8897286 [details] [diff] [review] Change NS_STYLE_IMAGELAYER_POSITION macro definitions to scoped enum class StyleImageLayerPosition Xidorn, do you mind taking this from my queue? Thanks.

Comment on attachment 8897286 [details] [diff] [review] Change NS_STYLE_IMAGELAYER_POSITION macro definitions to scoped enum class StyleImageLayerPosition Review of attachment 8897286 [details] [diff] [review]: ----------------------------------------------------------------- Please address the following comments and request review again with updated patch. Thanks for working on this. ::: layout/style/nsCSSParser.cpp @@ +10829,4 @@ > // using an unprefixed linear-gradient expression, so instead we'll just > // arbitrarily pretend this is a top-to-bottom gradient (which is the > // default for modern linear-gradient if a direction is unspecified). > + aGradient->mBgPos.mYValue.SetIntValue(int32_t(StyleImageLayerPosition::Bottom), You can use `SetEnumValue` instead. Its definition can be found here: https://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/layout/style/nsCSSValue.h#893-898 @@ +10833,2 @@ > eCSSUnit_Enumerated); > + aGradient->mBgPos.mXValue.SetIntValue(int32_t(StyleImageLayerPosition::Center), Ditto. @@ +10842,2 @@ > > int32_t targetX; Please use your new type StyleImageLayerPosition here rather than int32_t. It doesn't really make much sense to change it to enum class if we simply cast it to integer everywhere. @@ +10842,4 @@ > > int32_t targetX; > if (aStartPoint.mXValue == aEndPoint.mXValue) { > + targetX = int32_t(StyleImageLayerPosition::Center); With the type of this variable changes, we don't need the cast here. @@ +10846,3 @@ > } else if (IsWebkitGradientCoordLarger(aStartPoint.mXValue, > aEndPoint.mXValue)) { > + targetX = int32_t(StyleImageLayerPosition::Left); Ditto. @@ +10849,5 @@ > } else { > MOZ_ASSERT(IsWebkitGradientCoordLarger(aEndPoint.mXValue, > aStartPoint.mXValue), > "IsWebkitGradientCoordLarger returning inconsistent results?"); > + targetX = int32_t(StyleImageLayerPosition::Right); Ditto. @@ +10854,3 @@ > } > > int32_t targetY; And type here as well. @@ +10854,5 @@ > } > > int32_t targetY; > if (aStartPoint.mYValue == aEndPoint.mYValue) { > + targetY = int32_t(StyleImageLayerPosition::Center); Ditto. @@ +10859,3 @@ > } else if (IsWebkitGradientCoordLarger(aStartPoint.mYValue, > aEndPoint.mYValue)) { > + targetY = int32_t(StyleImageLayerPosition::Top); Ditto. @@ +10862,5 @@ > } else { > MOZ_ASSERT(IsWebkitGradientCoordLarger(aEndPoint.mYValue, > aStartPoint.mYValue), > "IsWebkitGradientCoordLarger returning inconsistent results?"); > + targetY = int32_t(StyleImageLayerPosition::Bottom); Ditto. @@ +11834,5 @@ > +#define BG_CENTER int32_t(StyleImageLayerPosition::Center) > +#define BG_TOP int32_t(StyleImageLayerPosition::Top) > +#define BG_BOTTOM int32_t(StyleImageLayerPosition::Bottom) > +#define BG_LEFT int32_t(StyleImageLayerPosition::Left) > +#define BG_RIGHT int32_t(StyleImageLayerPosition::Right) With the suggestion at the end of the patch (to make the enum bitwise-operable), these casts can be removed. @@ +12063,4 @@ > static nsCSSValue > BoxPositionMaskToCSSValue(int32_t aMask, bool isX) > { > + int32_t val = int32_t(StyleImageLayerPosition::Center); Please change the type here. @@ +12845,4 @@ > value->Item(1) = value->Item(0); // move xOffset to correct position > xEdge.Reset(); > } > + yEdge.SetIntValue(int32_t(StyleImageLayerPosition::Center), eCSSUnit_Enumerated); SetEnumValue here. @@ +12914,4 @@ > if (eCSSUnit_Enumerated == aEdge.GetUnit() && > eCSSUnit_Percent == aOffset.GetUnit()) { > switch (aEdge.GetIntValue()) { > + case int32_t(StyleImageLayerPosition::Center): Please instead cast the result of GetIntValue() in switch, so that compiler can check that we enumerate all properties here. Also, please use whitespaces rather than tabs to indent. @@ +12920,5 @@ > "center cannot be used with an offset"); > aOffset.SetPercentValue(0.5); > break; > + case int32_t(StyleImageLayerPosition::Bottom): > + MOZ_ASSERT(aDefaultEdge == uint8_t(StyleImageLayerPosition::Top)); Please change the type of `aDefaultEdge` parameter of this function to StyleImageLayerPosition as well, so that we can remove lots of casts. @@ +12957,4 @@ > // into the percentage with the default value in the edge. > // Offset lengths which are 0 can also be rewritten as 0% > AdjustEdgeOffsetPairForBasicShape(xEdge, xOffset, > + uint8_t(StyleImageLayerPosition::Left)); ... like this. ::: layout/style/nsRuleNode.cpp @@ +887,5 @@ > GetFloatFromBoxPosition(int32_t aEnumValue) > { > switch (aEnumValue) { > + case uint8_t(StyleImageLayerPosition::Left): > + case uint8_t(StyleImageLayerPosition::Top): Please use whitespaces rather than tabs. @@ +892,3 @@ > return 0.0f; > + case uint8_t(StyleImageLayerPosition::Right): > + case uint8_t(StyleImageLayerPosition::Bottom): The indention here looks incorrect. They should be unintend by one level. @@ +895,4 @@ > return 1.0f; > default: > MOZ_FALLTHROUGH_ASSERT("unexpected box position value"); > + case uint8_t(StyleImageLayerPosition::Center): Ditto. @@ +1545,4 @@ > DEFINE_ENUM_CLASS_SETTER(StyleFloat, None, InlineEnd) > DEFINE_ENUM_CLASS_SETTER(StyleFloatEdge, ContentBox, MarginBox) > DEFINE_ENUM_CLASS_SETTER(StyleHyphens, None, Auto) > + DEFINE_ENUM_CLASS_SETTER(StyleImageLayerPosition, Center, Right) I don't think this is necessary. StyleImageLayerPosition is a bit flag style enum... This setter macro wasn't designed for this kind of enum. Please remove this line. @@ +6951,5 @@ > > if (eCSSUnit_Enumerated == aEdge.GetUnit()) { > int sign; > + if (aEdge.GetIntValue() & (int32_t(StyleImageLayerPosition::Bottom) | > + int32_t(StyleImageLayerPosition::Right))) { Probably cast to int32_t after or'ed. ::: layout/style/nsStyleConsts.h @@ +342,4 @@ > > // See nsStyleImageLayers > // The parser code depends on |ing these values together. > +enum class StyleImageLayerPosition : uint8_t { This seems to be a bit flag enum. Could you use MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS to make it possible to be used with bitwise operators directly? There is one example for how this marco can be used: https://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/layout/style/ServoTypes.h#108-122 @@ +346,5 @@ > + Center = (1<<0), > + Top = (1<<1), > + Bottom = (1<<2), > + Left = (1<<3), > + Right = (1<<4) The parentheses in these lines are not necessary when not in macro. Please remove them. It would be better if you add whitespaces around "<<" as well.

Thank you for reviewing the patch. I have fixed most of the things but this part is giving me trouble... >@@ +11834,5 @@ > +#define BG_CENTER int32_t(StyleImageLayerPosition::Center) > +#define BG_TOP int32_t(StyleImageLayerPosition::Top) > +#define BG_BOTTOM int32_t(StyleImageLayerPosition::Bottom) > +#define BG_LEFT int32_t(StyleImageLayerPosition::Left) > +#define BG_RIGHT int32_t(StyleImageLayerPosition::Right) >With the suggestion at the end of the patch (to make the enum bitwise-operable), these casts can be removed. >... > >This seems to be a bit flag enum. Could you use MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS to make it possible to be used with bitwise >operators directly? There are multiple places where the enum is compared or bitwise operation is done with our enum class values and int. Such as in the below cases: > if ((xEdgeEnum | yEdgeEnum) == (BG_LEFT | BG_RIGHT) || > (xEdgeEnum | yEdgeEnum) == (BG_TOP | BG_BOTTOM) || > (xEdgeEnum & yEdgeEnum & ~BG_CENTER)) { > return false; > } and all of these lines throw-> >Error Comparison/Operation between int and enum class https://searchfox.org/mozilla-central/search?q=BG_CENTER and same for BG_TOP,BG_BOTTOM,BG_LEFT,BG_RIGHT. Which means that we still need to cast the values here. If thats the case then MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS doesn't seem to be much useful here. Is there something I am missing here?

Actually... after thinking a bit, I think we should probably not work on converting NS_STYLE_IMAGELAYER_POSITION macros. These macros are used in specified value only, which means they will be removed when we get rid of Gecko's style system after we start using Stylo everywhere. So there is little long-term benefit from doing conversion on this (since the code is expected to be removed in foreseeable future). Closing this as WONTFIX. You may want to pick some other macros to convert. Thanks for your contribution.