Closed Bug 1297982 Opened 4 years ago Closed 4 years ago

Replace NS_STYLE_BOX_* with enum classes

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: waffles, Assigned: waffles)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160801132323
Blocks: 1277133
Attachment #8784840 - Flags: review?(manishearth) → review?(xidorn+moz)
Attachment #8784841 - Flags: review?(manishearth) → review?(xidorn+moz)
Attachment #8784842 - Flags: review?(manishearth) → review?(xidorn+moz)
Attachment #8784843 - Flags: review?(manishearth) → review?(xidorn+moz)
Attachment #8784844 - Flags: review?(manishearth) → review?(xidorn+moz)
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.
Attachment #8784840 - Flags: review?(manishearth)
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.
Attachment #8784840 - Flags: review?(xidorn+moz) → review+
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
Attachment #8784841 - Flags: review?(xidorn+moz) → review+
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.
Attachment #8784842 - Flags: review?(xidorn+moz) → review+
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?
Attachment #8784843 - Flags: review?(xidorn+moz) → review+
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 { }?
Attachment #8784844 - Flags: review?(xidorn+moz) → review+
Attachment #8784840 - Flags: review?(manishearth)
Assignee: nobody → wafflespeanut
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
You need to log in before you can comment on or make changes to this bug.