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

Status: RESOLVED FIXED

(Core :: Widget, enhancement)

Description

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

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.

Comment 1

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

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.

Comment 2

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

And, actually, it would be better to use "enum class" rather than just "enum" as I used in the above patch.

Comment 3

[Paul Bignier]

Attachment: nsThemeConstants are enums instead of #defines. r=:dbaron

Comment 4

[Paul Bignier]

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

Comment 5

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

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

Comment 6

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

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

Comment 7

[Paul Bignier]

Attachment: nsThemeConstants are enums (ThemeWidgetType) instead of #defines. r=:dbaron

Comment 8

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

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"

Comment 9

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

Here's a try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c14930b94da300cd3f40915558cfdb570fcbee3&group_state=expanded

Comment 10

[Paul Bignier]

Attachment: change theme constants from #defines to enum ThemeWidgetType. r=:dbaron

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/659898a1c1b87949b3655d031342dd10d3160431
Bug 1367577 - change theme constants from #defines to enum ThemeWidgetType. r=dbaron

Comment 12

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

OK, landed on mozilla-inbound; it should merge to mozilla-central within a day or so.  Thanks for the patch!

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6037b902d6523133f8761dc3ad42cb9022fdd290
Backed out changeset 659898a1c1b8 (bug 1367577) for breaking stylo builds.

Comment 14

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

Bustage in stylo builds:
https://treeherder.mozilla.org/logviewer.html#?job_id=102015488&repo=mozilla-inbound

Comment 15

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

It's not clear to me if anything will work for stylo other than wholesale conversion to 'enum class'

Comment 16

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

Actually, I think it should be doable...

Comment 17

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

Attachment: Stylo changes for updated theme constants

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

Comment 18

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

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

Comment 19

[Bill Gianopoulos [:WG9s]]

This seems to me like the kind of thing that either should not have ever worked or someone implemented a really kludgey thing to make it work before.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 21

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

Try run that I forgot to link to earlier:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aed55800dbf06e516e672409f2eb1b5477324010&group_state=expanded

Comment 22

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

And here's a try push that should be new enough for regenerating the bindings:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82b70cbf0362406ab01f849f078fc9dd6aeffc5c&group_state=expanded

Comment 23

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

Attachment: link to Servo pull request

Comment 24

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

Attachment: Bug 1367577 - change theme constants from #defines to enum ThemeWidgetType.

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

Comment 25

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

Attachment: Bug 1367577 - Stylo changes for updated theme constants.

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.

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

Comment 26

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

Comment on attachment 8871558 [details]
Bug 1367577 - change theme constants from #defines to enum ThemeWidgetType.

https://reviewboard.mozilla.org/r/143042/#review146740

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8871559 [details]
Bug 1367577 - Stylo changes for updated theme constants.

https://reviewboard.mozilla.org/r/143044/#review146742

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b84dd92d6dc9
https://hg.mozilla.org/mozilla-central/rev/ad5da34b4bb3