Replace NS_STYLE_BOX_* with enum classes

RESOLVED FIXED in Firefox 51

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: waffles, Assigned: waffles)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

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

Updated

a year ago
Blocks: 1277133
(Assignee)

Comment 1

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6d61a3fd0bf
(Assignee)

Comment 2

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c361dde33c0
(Assignee)

Comment 3

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c11584a330b
(Assignee)

Comment 4

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5a2a8aab2dd
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 10

a year ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 31

a year ago
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

Comment 32

a year ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.