Closed
Bug 1297982
Opened 8 years ago
Closed 8 years ago
Replace NS_STYLE_BOX_* with enum classes
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: waffles, Assigned: waffles)
References
Details
Attachments
(5 files)
58 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160801132323
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•8 years ago
|
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•8 years 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)
Comment 11•8 years ago
|
||
Bouncing review to xidorn, he can review this. feedback+ from me. (Don't want to email-spam by adding individual feedback flags)
Comment 12•8 years ago
|
||
mozreview-review |
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 13•8 years ago
|
||
mozreview-review-reply |
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 14•8 years ago
|
||
mozreview-review-reply |
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 15•8 years ago
|
||
mozreview-review |
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 16•8 years ago
|
||
mozreview-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 17•8 years ago
|
||
mozreview-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 18•8 years ago
|
||
mozreview-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+
Updated•8 years ago
|
Attachment #8784840 -
Flags: review?(manishearth)
Updated•8 years ago
|
Assignee: nobody → wafflespeanut
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 19•8 years ago
|
||
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 25•8 years ago
|
||
mozreview-review |
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•8 years 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•8 years 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
Closed: 8 years 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.
Description
•