1403034 - Change color style define to enum class in nsStyleConsts.h

Status: RESOLVED DUPLICATE of bug 1563315

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

Description

[Leo Khodel [:lvk]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170816210634

Comment 1

[David Teller [:Yoric] - still alive but not very active]

File nsStyleColor.h ( http://searchfox.org/mozilla-central/source/layout/style/nsStyleConsts.h#237-246 ) defines a number of macros NS_COLOR_*. That's very fragile, so we should replace them with an `enum class`, which be just as efficient but will help us catch type errors.

The objective of this bug is to replace the definition of NS_COLOR_* with an `enum class` and to replace each instance of these color constants with their new value. See the other `enum class` definitions of the same file to see what the new `enum class` should look like.

Comment 2

[Leo Khodel [:lvk]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #1)
> File nsStyleColor.h (
> http://searchfox.org/mozilla-central/source/layout/style/nsStyleConsts.h#237-
> 246 ) defines a number of macros NS_COLOR_*. That's very fragile, so we
> should replace them with an `enum class`, which be just as efficient but
> will help us catch type errors.
> 
> The objective of this bug is to replace the definition of NS_COLOR_* with an
> `enum class` and to replace each instance of these color constants with
> their new value. See the other `enum class` definitions of the same file to
> see what the new `enum class` should look like.

Hi David, I intended for this bug to just concern NS_STYLE_COLOR_INHERIT_FROM_BODY. If it should include the NS_COLOR_* macros too, is it OK to combine the macros into one enum class called 'StyleColor'?

Comment 3

[David Teller [:Yoric] - still alive but not very active]

My bad, I had misunderstood your bug description. Also, I hadn't realized that you were the one working on it :)

I believe that yes, they should be combined together, as they are already used as a single enumeration.

Comment 4

[Leo Khodel [:lvk]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #3)
> My bad, I had misunderstood your bug description. Also, I hadn't realized
> that you were the one working on it :)

Sorry, should've been more explicit in the description :) Could you assign the bug to me?

> 
> I believe that yes, they should be combined together, as they are already
> used as a single enumeration.

Will do, thanks!

Comment 5

[Leo Khodel [:lvk]]

Forgot needinfo for comment #4.

Comment 6

[Leo Khodel [:lvk]]

Attachment: Bug 1403034: Convert NS_STYLE_COLOR.. and NS_COLOR_* macros to enum class.

Review commit: https://reviewboard.mozilla.org/r/184024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184024/

Comment 7

[Leo Khodel [:lvk]]

Attachment: Bug 1403034: Convert NS_STYLE_COLOR.. and NS_COLOR_* macros to enum class.

Review commit: https://reviewboard.mozilla.org/r/184036/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184036/

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Deflecting review to Manish.

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8912707 [details]
Bug 1403034: Convert NS_STYLE_COLOR.. and NS_COLOR_* macros to enum class.

https://reviewboard.mozilla.org/r/184036/#review190038

looks good, though I'll need to handle the servo side (this can't be landed directly)

Comment 10

[Leo Khodel [:lvk]]

(In reply to Manish Goregaokar [:manishearth] from comment #9)
> Comment on attachment 8912707 [details]
> Bug 1403034: Convert NS_STYLE_COLOR.. and NS_COLOR_* macros to enum class.
> 
> https://reviewboard.mozilla.org/r/184036/#review190038
> 
> looks good, though I'll need to handle the servo side (this can't be landed
> directly)

Great, thanks!

Comment 11

[Manish Goregaokar [:manishearth]]

The build doesn't pass, please fix all other usages of these macros.

Comment 12

[Leo Khodel [:lvk]]

Attachment: Bug 1403034: Fix constant uses.

Review commit: https://reviewboard.mozilla.org/r/186438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186438/

Comment 13

[Manish Goregaokar [:manishearth]]

Comment on attachment 8915199 [details]
Bug 1403034: Fix constant uses.

https://reviewboard.mozilla.org/r/186438/#review196978

Comment 14

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 15

[Emilio Cobos Álvarez (:emilio)]

I believe this was done in bug 1563315.