User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0 Build ID: 20160801132323

Comment on attachment 8784840 [details] Bug 1297982 - Replace NS_STYLE_BOX_ALIGN_* with enum class; https://reviewboard.mozilla.org/r/74144/#review72016 ::: layout/style/nsStyleConsts.h:172 (Diff revision 1) > #define NS_STYLE_WINDOW_DRAGGING_DEFAULT 0 > #define NS_STYLE_WINDOW_DRAGGING_DRAG 1 > #define NS_STYLE_WINDOW_DRAGGING_NO_DRAG 2 > > // box-align > -#define NS_STYLE_BOX_ALIGN_STRETCH 0 > +enum class StyleBoxAlign : uint8_t { Please move this to where the rest of the enum classes are, sorted alphabetically. Actually, come to think of it, I'm okay with it being left as-is and sorted once everything gets converted. Less churn that way. Other reviewers may want it to be at the top though, but hold off on moving this for now.

Bouncing review to xidorn, he can review this. feedback+ from me. (Don't want to email-spam by adding individual feedback flags)

Comment on attachment 8784840 [details] Bug 1297982 - Replace NS_STYLE_BOX_ALIGN_* with enum class; https://reviewboard.mozilla.org/r/74144/#review72194 r=me with the issue from Manish addressed.

Comment on attachment 8784840 [details] Bug 1297982 - Replace NS_STYLE_BOX_ALIGN_* with enum class; https://reviewboard.mozilla.org/r/74144/#review72016 > Please move this to where the rest of the enum classes are, sorted alphabetically. > > Actually, come to think of it, I'm okay with it being left as-is and sorted once everything gets converted. Less churn that way. Other reviewers may want it to be at the top though, but hold off on moving this for now. Actually I think it's okay as well. We should probably move others back, as I don't think having two separate lists helps anything.

Comment on attachment 8784840 [details] Bug 1297982 - Replace NS_STYLE_BOX_ALIGN_* with enum class; https://reviewboard.mozilla.org/r/74144/#review72016 > Actually I think it's okay as well. We should probably move others back, as I don't think having two separate lists helps anything. But I cannot close the issue, unfortunately. Ravi, you can drop this issue directly.

Comment on attachment 8784841 [details] Bug 1297982 - Replace NS_STYLE_BOX_PACK_* with enum class; https://reviewboard.mozilla.org/r/74146/#review72200

Comment on attachment 8784842 [details] Bug 1297982 - Replace NS_STYLE_BOX_DECORATION_BREAK_* with enum class; https://reviewboard.mozilla.org/r/74148/#review72202 ::: layout/generic/nsInlineFrame.cpp:140 (Diff revision 1) > StyleBorder()->mBoxDecorationBreak == > - NS_STYLE_BOX_DECORATION_BREAK_SLICE) { > + StyleBoxDecorationBreak::Slice) { You can actually merge these two lines now. ::: layout/style/nsStyleStruct.h:1335 (Diff revision 1) > - uint8_t mBoxDecorationBreak; // [reset] see nsStyleConsts.h > + // [reset] see nsStyleConsts.h > + mozilla::StyleBoxDecorationBreak mBoxDecorationBreak; Keep [reset] and drop "see nsStyleContsts.h" then merge the two lines like before, I think. Since we now use enum class which shows the possible values clearly itself, the additional pointer doesn't add any information.

Comment on attachment 8784843 [details] Bug 1297982 - Replace NS_STYLE_BOX_DIRECTION_* with enum class; https://reviewboard.mozilla.org/r/74150/#review72204 ::: layout/style/nsStyleStruct.h:3441 (Diff revision 1) > - uint8_t mBoxDirection; // [reset] see nsStyleConsts.h > + // [reset] see nsStyleConsts.h > + mozilla::StyleBoxDirection mBoxDirection; Drop "see nsStyleConsts.h" and merge the two lines. ::: layout/xul/nsBoxFrame.cpp:492 (Diff revision 1) > - if (boxInfo->mBoxDirection == NS_STYLE_BOX_DIRECTION_REVERSE) > + if (boxInfo->mBoxDirection == StyleBoxDirection::Reverse) > aIsNormal = !aIsNormal; // Invert our direction. As far as you're here, could you wrap the single line consequent of the if statement with { } and remove the whitespaces in the line after that single line to follow the coding style?

Comment on attachment 8784844 [details] Bug 1297982 - Replace NS_STYLE_BOX_ORIENT_* with enum class; https://reviewboard.mozilla.org/r/74152/#review72210 ::: layout/xul/nsBoxFrame.cpp:460 (Diff revision 1) > - if (boxInfo->mBoxOrient == NS_STYLE_BOX_ORIENT_HORIZONTAL) > + if (boxInfo->mBoxOrient == StyleBoxOrient::Horizontal) > aIsHorizontal = true; > else > aIsHorizontal = false; As far as you are here, could you wrap the single-line consequents of if and else here with { }?

The existing macros aren't sorted, so I thought it should be a good opportunity to gradually sort them, after converting them. It's not terribly important, though.

Comment on attachment 8784843 [details] Bug 1297982 - Replace NS_STYLE_BOX_DIRECTION_* with enum class; https://reviewboard.mozilla.org/r/74150/#review72336 ::: layout/xul/nsBoxFrame.cpp:495 (Diff revision 2) > // Now check the style system to see if we should invert aIsNormal. > const nsStyleXUL* boxInfo = StyleXUL(); > - if (boxInfo->mBoxDirection == NS_STYLE_BOX_DIRECTION_REVERSE) > + if (boxInfo->mBoxDirection == StyleBoxDirection::Reverse) { > aIsNormal = !aIsNormal; // Invert our direction. > + } > Please remove the whitespaces in this line.

Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d455dc75d677 Replace NS_STYLE_BOX_ALIGN_* with enum class; r=xidorn https://hg.mozilla.org/integration/autoland/rev/301caf87d081 Replace NS_STYLE_BOX_PACK_* with enum class; r=xidorn https://hg.mozilla.org/integration/autoland/rev/5d179cfa9eb1 Replace NS_STYLE_BOX_DECORATION_BREAK_* with enum class; r=xidorn https://hg.mozilla.org/integration/autoland/rev/d194c5416a37 Replace NS_STYLE_BOX_DIRECTION_* with enum class; r=xidorn https://hg.mozilla.org/integration/autoland/rev/4bd1109964d2 Replace NS_STYLE_BOX_ORIENT_* with enum class; r=xidorn

https://hg.mozilla.org/mozilla-central/rev/d455dc75d677 https://hg.mozilla.org/mozilla-central/rev/301caf87d081 https://hg.mozilla.org/mozilla-central/rev/5d179cfa9eb1 https://hg.mozilla.org/mozilla-central/rev/d194c5416a37 https://hg.mozilla.org/mozilla-central/rev/4bd1109964d2