1223653 - use enum class for computed values of box-sizing

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In bug 1223568 comment 0, Jesse suggested:
> (Should NS_STYLE_JUSTIFY_* and the other things in nsStyleConsts.h be enum
> classes, so they can have compiler-checked exhaustive switches?)

I think this is a good idea.  It also gets us additional type checking that we don't currently have (and don't get with regular enums, but do get with enum classes).

I decided to try it for box-sizing to see what it would look like.  We should probably discuss whether we like the style before continuing.

Interestingly, it caught a real bug where we were calling the wrong function.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The bug caught is:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/7a10f278c153/fix-wrong-compute-bsize
and a patch that compiles to convert box-sizing is:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/7a10f278c153/style-enum-class-box-sizing

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Good try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=26c5cf8c22e7&group_state=expanded

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 - Fix incorrect function being called, caught by enum class type checking in next patch

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 - Use an enum class for NS_STYLE_BOX_SIZING_*

The casts in nsCSSProps.cpp (defining kBoxSizingKTable) and in
nsComputedDOMStyle::DoGetBoxSizing (using
nsCSSProps::ValueToKeywordEnum) are a little bit annoying, though aren't
a net reduction in typesafety.

The casts in nsRuleNode.cpp (SetDiscrete) are a little more annoying,
though the change in this patch should be sufficient for converting all
properties -- but that may also mean reducing typesafety a bit for all
properties.

I'd like to find something better to do about them, but I think I'm ok
landing this before doing that.  Bug 1224918 covers doing better.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And it looks like the bug was a regression from the way I addressed review comments in bug 311415 comment 11 (see "I just added another variant of ComputeWidthValue").

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8687680 [details] [diff] [review]
patch 1 - Fix incorrect function being called, caught by enum class type checking in next patch

Review of attachment 8687680 [details] [diff] [review]:
-----------------------------------------------------------------

Ouch.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8687681 [details] [diff] [review]
patch 2 - Use an enum class for NS_STYLE_BOX_SIZING_*

Review of attachment 8687681 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsRuleNode.cpp
@@ +7856,5 @@
>                pos->mBoxSizing, conditions,
>                SETDSC_ENUMERATED | SETDSC_UNSET_INITIAL,
>                parentPos->mBoxSizing,
> +              StyleBoxSizing::Content,
> +              StyleBoxSizing::Content /* ignored */,

Do you think it would be more or less clear to make these ignored values StyleBoxSizing(0) rather than a specific named StyleBoxSizing value?

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #4)
> Created attachment 8687681 [details] [diff] [review]
> patch 2 - Use an enum class for NS_STYLE_BOX_SIZING_*
> 
> The casts in nsCSSProps.cpp (defining kBoxSizingKTable) and in
> nsComputedDOMStyle::DoGetBoxSizing (using
> nsCSSProps::ValueToKeywordEnum) are a little bit annoying, though aren't
> a net reduction in typesafety.
> 
> The casts in nsRuleNode.cpp (SetDiscrete) are a little more annoying,
> though the change in this patch should be sufficient for converting all
> properties -- but that may also mean reducing typesafety a bit for all
> properties.

There is a solution for the SetDiscrete case:

First, add two helper functions like
> template<typename Dest, typename Src>
> inline static void
> SetValue(Dest* aDest, const Src& aSrc)
> {
>   *aDest = aSrc;
> }
> template<typename Dest>
> inline static void
> SetValue(Dest* aDest, nullptr_t)
> {
>   MOZ_ASSERT_UNREACHABLE("inappropriate unit");
> }

Second, replace all the
> aField = aXXXValue;
to
> SetValue(&aField, aXXXValue);

Finally, make all ignored fields to pass "nullptr" instead of 0.

This should be typesafe for fields we change to use enum class, and doesn't affect anything we don't touch. No callsite change is needed for doing this, which means it could be in its own patch.

With this, once we convert all fields to use enum class, we can probably even remove aMask from SetDiscrete without weakening runtime assertion check.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #8)
> > template<typename Dest>
> > inline static void
> > SetValue(Dest* aDest, nullptr_t)

Errr, "decltype(nullptr)" instead of "nullptr_t".

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8687681 [details] [diff] [review]
patch 2 - Use an enum class for NS_STYLE_BOX_SIZING_*

Review of attachment 8687681 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleConsts.h
@@ +56,5 @@
>  #define NS_SIDE_TO_HALF_CORNER(side_, second_, parallel_) \
>    ((((side_) + !!(second_))*2 + ((side_) + !(parallel_))%2) % 8)
>  
>  // box-sizing
> +namespace mozilla {

I suppose you can probably just wrap the whole nsStyleConsts.h in "namespace mozilla", because macros are not affected by namespace.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) (away Nov 23 – Dec 4) from comment #7)
> Do you think it would be more or less clear to make these ignored values
> StyleBoxSizing(0) rather than a specific named StyleBoxSizing value?

I don't think it makes much difference in clarity, but the initial value seemed safer.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #8)
> With this, once we convert all fields to use enum class, we can probably
> even remove aMask from SetDiscrete without weakening runtime assertion check.

We would still need aMask for ENUMERATED/INTEGER and UNSET_INHERIT/INITIAL anyway.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73e519e9ec5e8a458d95dcf2ef6bda5ccb232cd4
Bug 1223653 patch 1 - Fix incorrect function being called, caught by enum class type checking in next patch.  r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/965b8e749d1f06b34ec176e4a7ddc5dcfd444e6e
Bug 1223653 patch 2 - Use an enum class for NS_STYLE_BOX_SIZING_*.  r=heycam

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73e519e9ec5e
https://hg.mozilla.org/mozilla-central/rev/965b8e749d1f