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)
Tracking
()
People
(Reporter: dbaron, Unassigned)
References
(Blocks 2 open bugs)
Details
(Keywords: good-first-bug, Whiteboard: [lang=c++][see comment 10])
Attachments
(1 file)
(deleted),
patch
|
xidorn
:
feedback+
|
Details | Diff | Splinter Review |
Comment 1•8 years ago
|
||
Comment 2•8 years ago
|
||
Comment 3•8 years ago
|
||
Comment 4•8 years ago
|
||
Comment 5•8 years ago
|
||
Comment 6•8 years ago
|
||
Comment 7•8 years ago
|
||
Comment 8•8 years ago
|
||
Comment 9•8 years ago
|
||
Comment 10•8 years ago
|
||
Comment 11•8 years ago
|
||
Comment 12•8 years ago
|
||
Comment 13•8 years ago
|
||
Comment 14•8 years ago
|
||
Comment 15•7 years ago
|
||
Updated•7 years ago
|
Comment 16•7 years ago
|
||
Comment 17•7 years ago
|
||
Comment 18•7 years ago
|
||
Reporter | ||
Comment 19•7 years ago
|
||
Updated•7 years ago
|
Comment 20•7 years ago
|
||
Comment 21•7 years ago
|
||
Comment 22•7 years ago
|
||
Comment 23•7 years ago
|
||
Comment 24•6 years ago
|
||
Comment 25•6 years ago
|
||
Comment 26•6 years ago
|
||
Updated•6 years ago
|
Comment 27•6 years ago
|
||
Comment 28•6 years ago
|
||
Comment 29•6 years ago
|
||
Comment 30•6 years ago
|
||
Comment 31•6 years ago
|
||
Comment 32•6 years ago
|
||
Comment 33•6 years ago
|
||
Comment 34•6 years ago
|
||
Comment 35•6 years ago
|
||
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•6 years ago
|
||
(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•6 years ago
|
||
Can I work on this one?
Comment 38•5 years ago
|
||
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•5 years ago
|
||
(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•5 years ago
|
||
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•5 years ago
|
||
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:
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•5 years ago
|
||
Can I work on this bug?
Comment 43•5 years ago
|
||
I would love to work on this bug, is it still available?
Comment 44•5 years ago
|
||
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•5 years ago
|
||
I'm not sure what the current status of this is, I suspect all the properties have been converted already?
Comment 46•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 47•5 years ago
|
||
It seems there's still some stuff to do related to this bug? I'd be happy to help out.
Comment 48•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
I see, thanks. Already had a look at those and raised a new bug.
Comment 52•5 years ago
|
||
In continuing on this task, could I just go ahead, select another constant, raise a bug and eventually submit a patch?
Comment 54•4 years ago
|
||
Hi, is this bug still open? If so, I'd like to help. Thanks!
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 55•2 years ago
|
||
Hey,
I'd like to migrate some defines. emilio, can I add you as a reviewer?
Updated•2 years ago
|
Comment 57•2 years ago
|
||
Hello, is this bug still open? I would like to help on it.
Comment 58•2 years ago
|
||
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•2 years ago
|
||
Thanks a lot Ben! \o/
Updated•2 years ago
|
Description
•