Closed Bug 1264238 Opened 5 years ago Closed 5 years ago

sort nsCSSPropList.h entries

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: heycam, Assigned: heycam)

Details

Attachments

(2 files)

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.
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: nobody → cam
Status: NEW → ASSIGNED
Attachment #8740873 - Flags: review?(dholbert)
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)
Attachment #8740873 - Flags: review?(dholbert) → review+
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/e79a7ee93d9c
https://hg.mozilla.org/mozilla-central/rev/04af26577b0d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.