1277133 - use enum classes for CSS property enumerated value constants (people interested in this bug: see comment 10 and comments 18-21)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

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

We should continue the work started in bug 1223653 and use enum classes for CSS property constants, for improved type safety.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

I can continue working on bug 1224918 and submit a patchset to make things work better in nsRuleNode. But I haven't figured out a good way to convert keyword tables to be generic. We can probably open another bug for that.

Ideally, I think keyword tables should be a class like:
> template<typename T>
> class KTable
> {
>   nsCSSKeyword mKeywords[];
>   T mValues[];
> };
So that value type (usually just 8bit) does not need to align with nsCSSKeyword (which is 16bit), and searching through either values or keywords would have a better locality thus improved performance (though the benefit would likely be marginal.)

But I cannot find out a way to accept keyword-value pairs and convert them to the form above in compile time. (Rust macro can really achieve this, but not C++.) It could be done via using an addition script phase, but I'm not sure whether it's worth...

Probably we should just make KTableEntry generic and simply ignore the alignment problem given it preexists... (I forget what was the problem with converting KTableEntry to be generic... I think there was some...)

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #1)
> Probably we should just make KTableEntry generic and simply ignore the
> alignment problem given it preexists... (I forget what was the problem with
> converting KTableEntry to be generic... I think there was some...)

Oh, I see what the problem would be in bug 1224918 comment 3 :)

Comment 3

[Cameron McCormack (:heycam)]

Attachment: allow KTableEntry to be initialized with enums

idorn, what do you think about this, to make the nsCSSProps.cpp entries easier and safer to initialize?  (Relies on mozilla::UnderlyingType, which I have a patch for too.)

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8773707 [details] [diff] [review]
allow KTableEntry to be initialized with enums

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

OK, this is probably the easiest way to go.

FWIW, the trait can be simplified to something like
> template<typename Enum, typename Storage>
> struct IsEnumFittingWithin
> {
>   static constexpr bool value = std::numeric_limits<Storage>::digits >=
>     std::numeric_limits<typename std::underlying_type<Enum>::type>::digits;
> };

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Or move the bool expression into the template argument of IntegralConstant...

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> FWIW, the trait can be simplified to something like
> > template<typename Enum, typename Storage>
> > struct IsEnumFittingWithin
> > {
> >   static constexpr bool value = std::numeric_limits<Storage>::digits >=
> >     std::numeric_limits<typename std::underlying_type<Enum>::type>::digits;
> > };

I guess this would allow |enum A : uint8_t| to fit within an int8_t storage, though I'm not sure we want to allow that.  Maybe checking min() / max() values would be a more obvious way to check than what I did in my patch.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Cameron McCormack (:heycam) from comment #6)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> > FWIW, the trait can be simplified to something like
> > > template<typename Enum, typename Storage>
> > > struct IsEnumFittingWithin
> > > {
> > >   static constexpr bool value = std::numeric_limits<Storage>::digits >=
> > >     std::numeric_limits<typename std::underlying_type<Enum>::type>::digits;
> > > };
> 
> I guess this would allow |enum A : uint8_t| to fit within an int8_t storage,
> though I'm not sure we want to allow that.  Maybe checking min() / max()
> values would be a more obvious way to check than what I did in my patch.

It shouldn't: http://en.cppreference.com/w/cpp/types/numeric_limits/digits

Checking min() / max() is also ok as far as it is only used with integers. Using numeric_limits::digits would additionally allow extending that to floating-points, which isn't very useful here, though.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #7)
> (In reply to Cameron McCormack (:heycam) from comment #6)
> > (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> > > FWIW, the trait can be simplified to something like
> > > > template<typename Enum, typename Storage>
> > > > struct IsEnumFittingWithin
> > > > {
> > > >   static constexpr bool value = std::numeric_limits<Storage>::digits >=
> > > >     std::numeric_limits<typename std::underlying_type<Enum>::type>::digits;
> > > > };
> > 
> > I guess this would allow |enum A : uint8_t| to fit within an int8_t storage,
> > though I'm not sure we want to allow that.  Maybe checking min() / max()
> > values would be a more obvious way to check than what I did in my patch.
> 
> It shouldn't: http://en.cppreference.com/w/cpp/types/numeric_limits/digits

Oops, meant to write that around the other way: allowing |enum A : int8_t| to fit within uint8_t, since digits() for the former will be 7 and 8 for the latter.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Hmmm, yeah, you're right. I guess we don't want to allow that. And checking min() / max() and integrality is probably the most precise way. And that way, I'd suggest using static_assert() inside the struct body to check whether the types are enum and integer, rather than relying on error after SFINAE. The error from the later is much more confusing than a straightforward static_assert().

Comment 10

[Manish Goregaokar [:manishearth]]

Marking as easy bug, willing to mentor.

This bug involves taking the `#define`d constants from nsStyleConsts.h (https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleConsts.h#117), and making them into enum classes instead.

You can find example patches for this in https://bugzilla.mozilla.org/show_bug.cgi?id=1288383. A relatively straightforward one is https://reviewboard.mozilla.org/r/66042/diff/11#index_header

Some things to watch out for:

 - In many places you may have to use `mozilla::StyleFooBar` instead of `StyleFooBar`. Please omit the `mozilla::` if there is a `using namespace mozilla` or `namespace mozilla{` at the top of the file.

 - In case one of the enum variants is named None, call it None_ instead

 - While the linked patches cast the enums to `uint8_t` in nsCSSProps.cpp, that is no longer necessary.

 - Please don't upload patches to this bug itself, create a new bug (if you want you can link the patch here and I'll create an assigned bug for you). Try not to change too many enums at once, and preferably split it out so that one enum is changed per patch.

Please let me know if you have any questions!

Comment 11

[Ravi Shankar [:waffles]]

In case anyone wanna accelerate their "constants-replacing" process, I've written a python script for doing it - https://github.com/Wafflespeanut/scripts/blob/gh-pages/python/ns_style_consts.py (it also requires a nearby script - https://github.com/Wafflespeanut/scripts/blob/gh-pages/python/helpers.py for executing a subprocess)

I agree that it's unnecessarily complicated (I kept playing with it in my free time, just to make the problem more interesting than how it was previously). Now, it works for most of the basic constants in `nsStyleConsts.h`, except a few, which have values spanned over multiple lines. `hg grep` is quite slow in my machine, so I added DXR support (with the help of `dryscrape` module), but that's optional.

I do something like this.

`python ns_style_consts.py ns style will`

It matches over the lines containing `NS_STYLE_WILL_CHANGE_*`, and shows all about how things are gonna change. I review it once and rerun the command with a `-w` flag (and only then it actually makes changes to files). 

The script only matches the "directly visible constants" - it's not at all intelligent to know anything about the types in individual struct fields, function returns, variable comparisons, or our `DEFINE_ENUM_CLASS_SETTER` workaround. That said, it still saves a lot of copy-paste work :)

Comment 12

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

>  - In case one of the enum variants is named None, call it None_ instead

The above workaround in comment 10 is no longer needed (fixed by bug 1300337). We should just use None.

Comment 13

[grhegde09]

Is this being worked on? Can I take this up? I am new here. So I may need some help.

Comment 14

[Ravi Shankar [:waffles]]

Hi. This is a tracking bug. It can't be finished straightaway. We usually pick a set of constants (sharing the same prefix) and change them to an enum class (see the other bugs that have addressed this above). You can do the same. Feel free to ping us if you have any questions :)

Comment 15

[Jeff Hajewski]

Do we want to update all constants in nsStyleConsts.h? I was looking at a few this morning, for example NS_STYLE_AZIMUTH/ELEVATION/PITCH/SPEAK (lines 233 - 276) and they don't appear to occur in any files other than some pub enums in servo. On the other hand, something like NS_STYLE_IMAGELAYER_* is used more extensively (193 hits on DXR vs 36).

Comment 16

[akriti verma]

hi @dbaron , i would like to work on this bug. Can you give me some pointers on how to start.

Comment 17

[akriti verma]

hi Manish , i would like to get working on this bug. Can you give me some pointers on how to start.

Comment 18

[Jeff Hajewski]

(In reply to akriti verma from comment #17)
> hi Manish , i would like to get working on this bug. Can you give me some
> pointers on how to start.

I would start by looking through layout/style/nsStyleConsts.h [1] and picking a set of #define'd constants you would like to change to enum classes. Once you've settled on a set of constants, make the change throughout the codebase. Using a tool like grep or DXR [2] can be helpful in finding the locations of the constants you would like to update.

You can use the enum classes that are already there as a template.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleConsts.h#117
[2] https://dxr.mozilla.org/mozilla-central/source/

Comment 19

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

And this bug should be a metabug -- each actual conversion should be filed as a separate bug.  See the "Depends on:" list above for examples.

Comment 20

[akriti verma]

Do we need to change all the #define constants, because there will be many enum classes then , under different names which may affect other dependencies.

Comment 21

[Manish Goregaokar [:manishearth]]

Change one set of #define constants into one enum, and fix the fallout, yes. Don't fix all the #defines at once, only one group of them.

See bug 1369448 for an example.

Comment 22

[Ekta Tiwari]

I want to resolve to this issue.Please assign me

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Ekta Tiwari from comment #22)
> I want to resolve to this issue.Please assign me

See comment 18 and comment 19 for how to get started on a piece of this.  And see the closed bugs listed in the "Depends on:" field at the top of this bug for examples of patches for other already-fixed pieces, if that helps.

Comment 24

[Sidharth]

Hi, 
I would like to work on this bug. 
Can you assign this bug to me?

Comment 25

[Devika Sugathan]

Hello,
I would like to work on this bug.

Comment 26

[Daniel Holbert [:dholbert]]

(In reply to Sidharth from comment #24)
> Hi, 
> I would like to work on this bug. 
> Can you assign this bug to me?

(In reply to Devika Sugathan from comment #25)
> Hello,
> I would like to work on this bug.

Thanks! See comment 18 / comment 19 for how to get started.

Comment 27

[Ian Gralinski]

What testing should be performed when making these changes apart from ensuring Firefox still builds? Are there any specific unit tests, integration tests, etc.?

Comment 28

[Emilio Cobos Álvarez (:emilio)]

(In reply to Ian Gralinski from comment #27)
> What testing should be performed when making these changes apart from
> ensuring Firefox still builds? Are there any specific unit tests,
> integration tests, etc.?

Building is generally enough for these changes.

Comment 29

[Ian Gralinski]

I have created a new bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1487042) for part of this change. Can I add this to the list of bugs this Depends On?

Comment 30

[Tim B [:tb120]]

Created bug 1500439 to track NS_STYLE_ANIMATION_PLAY_STATE conversion.

Comment 31

[William Strode]

Hi,

I'm looking for a first bug to work - if possible could I be assigned to this (or some subtask of this if it has be redefined as a meta-bug since comments 18/19)

Thanks!

Comment 32

[shreyans]

Hi,
I am new to open source and don't know how to get started.
Please help me out here?

Comment 33

[sharath.savasere]

Hello,

I am new to open source (this is my first bug fixing for mozilla) and took sometime to go through this bug. I made changes to the code but it's failing to compile. Can I get some help regarding this. I am working on #define NS_STYLE_COLOR_ADJUST tag.
 

Thanks!

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Hi! Please file a new bug, and attach your patch there, I'm happy to take a look and help out with the build issue :)

Comment 35

[asfiyab95]

Hi, I am working on NS_STYLE_FLEX_DIRECTION. I have made changes to the code, but the build fails. Can someone please help me with this?

Comment 36

[Emilio Cobos Álvarez (:emilio)]

(In reply to asfiyab95 from comment #35)

Hi, I am working on NS_STYLE_FLEX_DIRECTION. I have made changes to the code, but the build fails. Can someone please help me with this?

Can you file a new bug for that and post there the build failure and the patch? I'm happy to take a look :)

Comment 37

[Vaishnavi Kaulagi]

Can I work on this one?

Comment 38

[Jeremy Ir]

I'm looking for some help on the changes in comment #38. I'm having some odd build issues. I've uploaded the patch and the output error. I'm unfamiliar with how to interact with Gecko, so I'm at a bit of an impass.

Comment 39

[Daniel Holbert [:dholbert]]

(In reply to Jeremy Ir from comment #38)

I'm looking for some help on the changes in comment #38. I'm having some odd build issues.

Sure -- I posted some suggestions on your bug 1548341.

Comment 40

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

Emilio, is there a simple way to tell whether a macro in nsStyleConsts.h is already implemented by servo and is suitable to be generated by cbindgen? We might want these candidates to be generated by cbindgen directly rather than just manually convert them to a c++ enum class.

Comment 41

[Emilio Cobos Álvarez (:emilio)]

Yes, I agree. I don't think there's a great way to do that short of looking at the property definition (it's trivial looking at the property name and adding a .mako.rs filter in Searchfox) like:

https://searchfox.org/mozilla-central/search?q=color-interpolation&case=false&regexp=false&path=mako.rs

There you can see whether it uses predefined_type or single_keyword. If the first, then we should just use cbindgen. I tend to think that for the second we should also do that, but that's a bit more work. In any case moving from #define to enum class makes eventually moving to cbindgen trivial (just a matter of removing a few lines), so it's not wasted effort.

Comment 42

[Harshit Babbar]

Can I work on this bug?

Comment 43

[sanchit.arora.2002]

I would love to work on this bug, is it still available?

Comment 44

[Karan]

From where do I start working on this bug? This is my first contribution to mozilla so not too sure where to begin with.

Comment 45

[Manish Goregaokar [:manishearth]]

I'm not sure what the current status of this is, I suspect all the properties have been converted already?

Comment 46

[Emilio Cobos Álvarez (:emilio)]

There's still stuff to do, not all of them have been converted, see this code and a bunch of related defines.

There are a few bugs like bug 1341018 that were pretty close to landing. I'll land that one. But the procedure is still the same from comment 18 / comment 21.

I've filed some bugs with some of the properties, but there are some others further down that file that also need the same treatment.

Comment 47

[Philipp Zech]

It seems there's still some stuff to do related to this bug? I'd be happy to help out.

Comment 48

[Martin McNickle]

Phillipp, yes there is. Have a look at the bugs I raised above, and raise one for one of the remaining consts in nsStyleConsts.h

I think the NS_STYLE_COLOR_INTERPOLATION_* consts would be a good one to start on. The comments and similar bugs above should give you a good idea of what needs to be done.

Comment 49

[Philipp Zech]

OK, gonna give it a shot. Just one quick question: which bugs do you mean and how do I raise one? I'm pretty new here...

Comment 50

[Emilio Cobos Álvarez (:emilio)]

The bugs Martin means are the ones he's fixed recently, they're in the comments above yours ("Depends on: 1612148" and so on).

In order to raise one you can use this link, which is really just from the New / Clone menu in the top of the page.

Comment 51

[Philipp Zech]

I see, thanks. Already had a look at those and raised a new bug.

Comment 52

[Philipp Zech]

In continuing on this task, could I just go ahead, select another constant, raise a bug and eventually submit a patch?

Comment 53

[Emilio Cobos Álvarez (:emilio)]

Yes, that sounds great!

Comment 54

[nhariish2000]

Hi, is this bug still open? If so, I'd like to help. Thanks!

Comment 55

[ben.freist]

Hey,
I'd like to migrate some defines. emilio, can I add you as a reviewer?

Comment 56

[Emilio Cobos Álvarez (:emilio)]

Yes please, feel free to do so. Thanks!

Comment 57

[Pascal Berndt]

Hello, is this bug still open? I would like to help on it.

Comment 58

[ben.freist]

The MR that removes the last relevant define from nsStyleConsts.h is going to land soon. I think this bug can be closed then.

Comment 59

[Emilio Cobos Álvarez (:emilio)]

Thanks a lot Ben! \o/