1264238 - sort nsCSSPropList.h entries

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

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.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Part 1: Remove unused CSS_PROP_STUB_NOT_CSS macro.

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.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 2: Sort nsCSSPropList.h entries.

I decided to keep the entries sorted regardless of #ifdefs around them, so a couple of #ifdef blocks have been broken up.

Comment 3

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41e97385321f

Comment 4

[Daniel Holbert [:dholbert]]

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

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e79a7ee93d9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/04af26577b0d

Comment 6

[Cameron McCormack (:heycam)]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e79a7ee93d9c
https://hg.mozilla.org/mozilla-central/rev/04af26577b0d