Closed
Bug 1264238
Opened 8 years ago
Closed 8 years ago
sort nsCSSPropList.h entries
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: heycam, Assigned: heycam)
Details
Attachments
(2 files)
1.58 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
92.51 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
From bug 1261754 comment 22. The entries in nsCSSPropList.h are kind of sorted, but not really. It would be nice to keep them sorted.
Assignee | ||
Comment 1•8 years ago
|
||
I think the intention was to have a CSS_PROP_STUB_NOT_CSS entry per property #ifndef CSS_PROP_LIST_EXCLUDE_INTERNAL, but it has been a while since that's been the case. We don't use CSS_PROP_STUB_NOT_CSS anywhere, so let's just remove it.
Assignee | ||
Comment 2•8 years ago
|
||
I decided to keep the entries sorted regardless of #ifdefs around them, so a couple of #ifdef blocks have been broken up.
Attachment #8740874 -
Flags: review?(dholbert)
Assignee | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41e97385321f
Updated•8 years ago
|
Attachment #8740873 -
Flags: review?(dholbert) → review+
Comment 4•8 years ago
|
||
Comment on attachment 8740874 [details] [diff] [review] Part 2: Sort nsCSSPropList.h entries. r=me. One opportunity for simplification, around the "mask" / "MOZ_ENABLE_MASK_AS_SHORTHAND" stuff: >+++ b/layout/style/nsCSSPropList.h [...] >+#ifdef MOZ_ENABLE_MASK_AS_SHORTHAND >+CSS_PROP_SHORTHAND( >+ mask, >+ mask, >+ Mask, >+ CSS_PROPERTY_PARSE_FUNCTION, >+ "") >+#else >+CSS_PROP_SVGRESET( >+ mask, [...] >+#endif // MOZ_ENABLE_MASK_AS_SHORTHAND >+#ifdef MOZ_ENABLE_MASK_AS_SHORTHAND >+CSS_PROP_SVGRESET( >+ mask-clip, [...] >+ kImageLayerSizeKTable, >+ CSS_PROP_NO_OFFSET, >+ eStyleAnimType_Custom) >+#endif // MOZ_ENABLE_MASK_AS_SHORTHAND If you like, you could simplify this a bit (collapsing the two separate #if checks into one) if you reverse the polarity of the first check. i.e.: #ifndef MOZ_ENABLE_MASK_AS_SHORTHAND CSS_PROP_SVGRESET( mask, [...] #else CSS_PROP_SHORTHAND( mask, [...] [...all mask-* subproperties go here, except mask-type...] #endif // MOZ_ENABLE_MASK_AS_SHORTHAND I'll grant that it's a bit counterintuitive to use "#ifndef...#else", but it makes this block as a whole much clearer IMO, since now the "mask" shorthand would get to actually be grouped with all of its subproperties, both visually and from an #if-scope perspective. (And it's still alphabetically sorted by property name; just the two ways of representing "mask" itself have been swapped.)
Attachment #8740874 -
Flags: review?(dholbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e79a7ee93d9c https://hg.mozilla.org/integration/mozilla-inbound/rev/04af26577b0d
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #4) > If you like, you could simplify this a bit (collapsing the two separate #if > checks into one) if you reverse the polarity of the first check. Nice idea, I've done this.
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e79a7ee93d9c https://hg.mozilla.org/mozilla-central/rev/04af26577b0d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•