Closed Bug 1367577 Opened 2 years ago Closed 2 years ago

nsThemeConstants should be an enum rather than #defines, and use consecutive values

Categories

(Core :: Widget, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dbaron, Unassigned, Mentored)

References

Details

(Whiteboard: [lang=c++][good sixth bug])

Attachments

(5 files, 2 obsolete files)

nsThemeConstants.h should be defining an enum (for better typesafety), and the values should be consecutive (so that theme implementations can cache in an array for things bug 1367576).

This is probably a pretty straightforward patch to write, but it's going to be a *large* patch.
A very basic start to what I was thinking was this patch:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ff9bef35f436/theme-types-enum
but the work to fix up the implementations and the callers is most of the work.
Mentor: dbaron
Whiteboard: [lang=c++][good sixth bug]
And, actually, it would be better to use "enum class" rather than just "enum" as I used in the above patch.
Hello David,

This first patch only turns defines into enums, it works easily and will serve as fallback.
The use of "enum class" indeed makes it a large commit with vast ramifications.
Atm I'm working on it but at some places I'm not even sure if I should extend the conversion to "Theme::" or simply cast the value to uint8_t!
I'll post a fix proposal so you can judge.

Best regards,
Paul
Hmmm.  That's a fair point that it can be converted to an enum rather than an enum class without fixing up as much.  So I think we should combine the approaches of the two patches:
 * skip the "namespace mozilla" that I added, since that requires fixing up the callers
 * add the include guards as you did, although as nsThemeConstants_h_
 * use the indentation from my patch, as well as the removal of the explicit constants (since using consecutive values is needed for bug 1367576)
 * use the ThemeWidgetType name that I used, rather than Theme, since Theme is a name that's more appropriate for nsITheme
(The point of converting to "enum class" would be to use that type everywhere it's used, and get rid of the use of uint8_t for the value.)
Attachment #8871219 - Attachment is obsolete: true
So that patch looks fine, except for:
 * it introduces two stray spaces in the license header, and a stray comma after "// The gripper for a toolbar."
 * the "No newline at end of file"
 * commit messages should generally describe change, not state, so the commit message should be more like "change theme constants from #defines to enum ThemeWidgetType"
Attachment #8871247 - Attachment is obsolete: true
OK, landed on mozilla-inbound; it should merge to mozilla-central within a day or so.  Thanks for the patch!
It's not clear to me if anything will work for stylo other than wholesale conversion to 'enum class'
Actually, I think it should be doable...
This is a little bit odd, because I didn't want to do all the work to
convert the theme constants to an enum class (although that would be
nice to do in the future).  So this makes them a regular enum, but still
uses gecko_constant_prefix rather than gecko_enum_prefix because that
makes it possible to add the NS_THEME_ prefix that's still present.

MozReview-Commit-ID: DtWYOF3wxXy
Attachment #8871418 - Flags: review?(xidorn+moz)
My understanding is that the way to land this is to:

 * make a PR to servo with the servo change in attachment 8871418 [details] [diff] [review] plus the regenerated bindings changes from a try build

 * make sure that PR merges at the same time that somebody can reland https://hg.mozilla.org/integration/mozilla-inbound/rev/659898a1c1b87949b3655d031342dd10d3160431, plus the layout/style changes from attachment 8871418 [details] [diff] [review], on autoland
(In reply to David Baron :dbaron: ⌚️UTC-4 from comment #18)
> My understanding is that the way to land this is to:
> 
>  * make a PR to servo with the servo change in attachment 8871418 [details] [diff] [review]
> [diff] [review] plus the regenerated bindings changes from a try build
> 
>  * make sure that PR merges at the same time that somebody can reland
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 659898a1c1b87949b3655d031342dd10d3160431, plus the layout/style changes from
> attachment 8871418 [details] [diff] [review], on autoland

Yes. These additional steps as well as coordination (especially considering the long wait time for servo CI these days) really make me unwilling to do this kind of change when not necessary before our tooling gets improved.

But there are patches... okay, I guess I can land it...
Attachment #8871418 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8871558 [details]
Bug 1367577 - change theme constants from #defines to enum ThemeWidgetType.

https://reviewboard.mozilla.org/r/143042/#review146740
Attachment #8871558 - Flags: review?(dbaron) → review+
Comment on attachment 8871559 [details]
Bug 1367577 - Stylo changes for updated theme constants.

https://reviewboard.mozilla.org/r/143044/#review146742
Attachment #8871559 - Flags: review?(xidorn+moz) → review+
Pushed by dbaron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b84dd92d6dc9
change theme constants from #defines to enum ThemeWidgetType. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/ad5da34b4bb3
Stylo changes for updated theme constants.  r=xidorn
https://hg.mozilla.org/mozilla-central/rev/b84dd92d6dc9
https://hg.mozilla.org/mozilla-central/rev/ad5da34b4bb3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.