Closed
Bug 1325006
Opened 8 years ago
Closed 8 years ago
Convert NS_RADIUS_* to an enum class
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla53
| Tracking | Status | |
|---|---|---|
| firefox53 | --- | fixed |
People
(Reporter: TYLin, Assigned: TYLin)
References
Details
Attachments
(2 files)
While implementing basic shape circle() value for shape-outside property, I feel that it might be worth to convert NS_RADIUS_* to an enum class.
http://searchfox.org/mozilla-central/rev/cc2a84852bd4e6f6d8d4d5b17b8382bb5d005749/layout/style/nsStyleConsts.h#222-223
| Comment hidden (mozreview-request) |
|
Assignee | |
Comment 2•8 years ago
|
||
Though it'll a bit awkward that we need to convert StyleShapeRadius back and forth at some places, it might not worth the effort to develop a general mechanism for nsStyleCoord to cooperates better with enum class. We only have a few usage of nsStyleCoord::SetIntValue() with eStyleUnit_Enumerated.
http://searchfox.org/mozilla-central/search?q=symbol:E_%3CT_nsStyleUnit%3E_eStyleUnit_Enumerated&redirect=false
|
||
Comment 3•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8820596 [details]
Bug 1325006 Part 2 - Convert NS_RADIUS_* to StyleShapeRadius enum class.
https://reviewboard.mozilla.org/r/100090/#review100556
::: layout/style/nsRuleNode.cpp:9758
(Diff revision 1)
> aStyleContext,
> aPresContext,
> aConditions);
> MOZ_ASSERT(didSetRadius, "unexpected radius unit");
> } else {
> - radius.SetIntValue(NS_RADIUS_CLOSEST_SIDE, eStyleUnit_Enumerated);
> + radius.SetIntValue(static_cast<int32_t>(StyleShapeRadius::ClosestSide),
I wonder if we should make SetIntValue templated, like the one on nsCSSValue, so we can avoid the cast here. And maybe also add some templated functions like:
template<typename T>
T GetIntValueAs() { return static_cast<T>(GetIntValue()); }
to nsCSSValue and nsStyleCoord, to avoid some more explicit casts.
Attachment #8820596 -
Flags: review?(cam) → review+
|
||
Comment 4•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8820596 [details]
Bug 1325006 Part 2 - Convert NS_RADIUS_* to StyleShapeRadius enum class.
https://reviewboard.mozilla.org/r/100090/#review100564
::: layout/style/nsRuleNode.cpp:9758
(Diff revision 1)
> aStyleContext,
> aPresContext,
> aConditions);
> MOZ_ASSERT(didSetRadius, "unexpected radius unit");
> } else {
> - radius.SetIntValue(NS_RADIUS_CLOSEST_SIDE, eStyleUnit_Enumerated);
> + radius.SetIntValue(static_cast<int32_t>(StyleShapeRadius::ClosestSide),
It would be good to have something like `template<typename T> void SetEnumValue(T aValue);`
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 7•8 years ago
|
||
heycam, xidorn, thank you for the suggestion. It turns out that it's not hard to allow enum to be stored in nsStyleCoord.
|
|
||
Comment 8•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8820620 [details]
Bug 1325006 Part 1 - Allow enum or enum classes to be stored in nsStyleCoord.
https://reviewboard.mozilla.org/r/100104/#review100608
Looks good, thanks!
Attachment #8820620 -
Flags: review?(cam) → review+
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f96cde4685a2
Part 1 - Allow enum or enum classes to be stored in nsStyleCoord. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b81c89d068a7
Part 2 - Convert NS_RADIUS_* to StyleShapeRadius enum class. r=heycam
|
||
Comment 10•8 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/f96cde4685a2
https://hg.mozilla.org/mozilla-central/rev/b81c89d068a7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•