Closed Bug 1223653 Opened 4 years ago Closed 4 years ago

use enum class for computed values of box-sizing

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
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.
Attachment #8687681 - Flags: review?(cam)
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 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.
Attachment #8687680 - Flags: review?(jfkthame) → review+
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?
Attachment #8687681 - Flags: review?(cam) → review+
(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.
(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 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.
(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.
(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.
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
https://hg.mozilla.org/mozilla-central/rev/73e519e9ec5e
https://hg.mozilla.org/mozilla-central/rev/965b8e749d1f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.