Open Bug 1277133 Opened 4 years ago Updated 1 month ago

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

Categories

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

defect

Tracking

()

People

(Reporter: dbaron, Unassigned, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=c++][see comment 10])

Attachments

(1 file)

We should continue the work started in bug 1223653 and use enum classes for CSS property constants, for improved type safety.
Depends on: 1224918
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...)
(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 :)
Depends on: 1277773
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.)
Attachment #8773707 - Flags: feedback?(xidorn+moz)
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;
> };
Attachment #8773707 - Flags: feedback?(xidorn+moz) → feedback+
Or move the bool expression into the template argument of IntegralConstant...
Depends on: 1289033
(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.
(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.
(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.
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().
Depends on: 1289710
Depends on: 1290023
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!
Mentor: manishearth
Keywords: good-first-bug
Whiteboard: [lang=c++][see comment 10]
Depends on: 1291667
Depends on: 1293604
Depends on: 1297982
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 :)
Depends on: 1312173
>  - 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.
Depends on: 1313565
Depends on: 1325006
Is this being worked on? Can I take this up? I am new here. So I may need some help.
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 :)
Depends on: 1325940
Depends on: 1326125
Depends on: 1339765
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).
Priority: -- → P3
hi @dbaron , i would like to work on this bug. Can you give me some pointers on how to start.
hi Manish , i would like to get working on this bug. Can you give me some pointers on how to start.
Flags: needinfo?(manishearth)
(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/
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.
Flags: needinfo?(manishearth)
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.
Flags: needinfo?(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.
Flags: needinfo?(manishearth)
Depends on: 1389862
Depends on: 1390400
Depends on: 1403034
Depends on: 1404237
I want to resolve to this issue.Please assign me
Flags: needinfo?(manishearth)
(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.
Flags: needinfo?(manishearth)
Hi, 
I would like to work on this bug. 
Can you assign this bug to me?
Depends on: 1459363
Depends on: 1459367
Depends on: 1460439
Hello,
I would like to work on this bug.
(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.
Summary: use enum classes for CSS property enumerated value constants → use enum classes for CSS property enumerated value constants (people interested in this bug: see comment 10 and comments 18-21)
What testing should be performed when making these changes apart from ensuring Firefox still builds? Are there any specific unit tests, integration tests, etc.?
(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.
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?
Depends on: 1487042
Created bug 1500439 to track NS_STYLE_ANIMATION_PLAY_STATE conversion.
Depends on: 1509664
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!
Hi,
I am new to open source and don't know how to get started.
Please help me out here?
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!
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 :)
Depends on: 1526447

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?

(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 :)

Can I work on this one?

Depends on: 1548341

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.

(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.

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.

Flags: needinfo?(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.

Flags: needinfo?(emilio)

Can I work on this bug?

Depends on: 1601990
Depends on: 1597741
Depends on: 1597979
Depends on: 1597881
Depends on: 1598539
Depends on: 1597893
Depends on: 1598348
Depends on: 1598528
Depends on: 1600476
Depends on: 1600483
Depends on: 1600484
Depends on: 1600482
Depends on: 1600481
Depends on: 1601856

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

Flags: needinfo?(manishearth)

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

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

Flags: needinfo?(manishearth) → needinfo?(emilio)
Depends on: 1611040
Depends on: 1611041
Depends on: 1611043
Depends on: 1611044

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.

Flags: needinfo?(emilio)
Depends on: 1611733
Depends on: 1611829
Depends on: 1612143
Depends on: 1612146
Depends on: 1612148

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

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.

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...

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.

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

Depends on: 1617644

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

Flags: needinfo?(emilio)

Yes, that sounds great!

Flags: needinfo?(emilio)
Depends on: 1622332
Depends on: 1620034
Depends on: 1621706
Depends on: 1625745
Depends on: 1625699

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

You need to log in before you can comment on or make changes to this bug.