1325006 - Convert NS_RADIUS_* to an enum class

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1325006 Part 2 - Convert NS_RADIUS_* to StyleShapeRadius enum class.

The enum value of nsStyleCoord is stored in int32_t, so it'll a bit awkward
that we need to convert StyleShapeRadius either from or to integer type.
However, it still worth doing because other utility functions will still
benefit from the type-safe enum class.

For example, the "default" case in EnumerationToLength() is not needed
anymore because StyleShapeRadius is an enum class, which cannot have other
values.

Review commit: https://reviewboard.mozilla.org/r/100090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100090/

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

[Cameron McCormack (:heycam)]

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.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

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 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1325006 Part 1 - Allow enum or enum classes to be stored in nsStyleCoord.

Review commit: https://reviewboard.mozilla.org/r/100104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100104/

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8820596 [details]
Bug 1325006 Part 2 - Convert NS_RADIUS_* to StyleShapeRadius enum class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100090/diff/1-2/

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

[Cameron McCormack (:heycam)]

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!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f96cde4685a2
https://hg.mozilla.org/mozilla-central/rev/b81c89d068a7