1210905 - In property_database.js (for mochitests), make alias properties copy their values from alias-target, instead of duplicating lines

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Right now in property_database.js, we duplicate initial_values/other_values/invalid_values in property aliases.

This is particularly long in aliases for complex properties like "-moz-transform" and "-moz-transition".

To avoid this duplication & increase maintainability (and make it easier to add tests for new aliases), we should just do this copying automatically.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: make alias properties copy values fields from their alias-targets

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8669094 [details] [diff] [review]
part 1: make alias properties copy values fields from their alias-targets

[actually, tweaking this slightly; will repost for review after I'm back from lunch]

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 1 v2: make aliases copy "*_values" fields from their alias-targets

This version copies other less-used "values" fields as well, if present.  I also updated the documentation in property_database.js to note that these fields should be left unset.

("part 2", coming up, will actually remove these fields from existing aliases in gCSSProperties.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 2: remove *_values fields from existing aliases

This patch adjusts the actual alias entries in gCSSProperties to remove their *_values fields (since we'll be stomping on them now).

This also upgrades a "todo" from the previous patch to an "ok", now that we can legitimately assume that these fields are not present (until we set them programmatically).

Comment 5

[Daniel Holbert [:dholbert]]

The "invalid_values.push" line-removal, towards the end of Part 2, is necessary because invalid_values does not exist at this point for these aliased properties. [it gets set up lower down]

In most cases, we were already performing the same push() on the alias targets, so just dropping the lines should be fine. (because they'll end up cloning the alias targets values-lists after this point, anyway).  In 2 of the cases (for "-moz-animation" & "-moz-animation-direction"), we weren't making the exact same tweak to the alias-target property, so I changed it so that now we do.

Comment 6

[Daniel Holbert [:dholbert]]

Just noticed that I left out a commit message in part 2. Commit message is:
 Bug 1210905 part 2: Remove now-unneeded *_values fields from aliases in property_database.js. r=heycam

Comment 7

[Daniel Holbert [:dholbert]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d6ef49ba9fc

Comment 8

[Daniel Holbert [:dholbert]]

Ah, Try run shows that we have some "/* valid only when prefixed */" styles for -moz-transform.  (Looks like this is a special-case hack, to preserve some legacy behavior that Bing Maps relied on, after a spec-change; see bug 774169 comment 24.)

So, I need to change part 1 here to give us the *option* of including *_values fields for alias properties [and avoid stomping on them if we do specify values].

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: (wrong patch file; ignore)

Here's an updated version of part 1, now being more graceful & not stomping on aliases' values lists if they're present.

(This means we can leave "-moz-transform" with its own custom lists in part 2, and be confident that they'll be respected.)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: part 1 v3: make aliases copy "*_values" fields from their alias-targets

[sorry, that was a patch from a different bug; here's the correct updated patch]

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: part 2, v2: remove *_values fields from aliases

...and here's an updated version of part 2, leaving "-moz-transform" with its own custom other_values & invalid_values intact now (but dropping those fields from all other aliases)

Comment 12

[Daniel Holbert [:dholbert]]

New Try run looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff4a18832831

Comment 13

[Daniel Holbert [:dholbert]]

Sorry, making one more tweak here -- I realized I might as well clone the "prerequisites" field, too, because the alias-target's prereqs [if any] should be good enough for the alias as well.  This lets us remove some more duplicated code and makes adding new aliases a bit easier still.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: part 1 v4: Make property_database.js automatically populate aliasing properties' *_values & prerequisites fields based on their alias target

(just updating to include "prerequisites" as noted in comment 13)

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: part 2 v3: Remove now-unneeded *_values & prerequisites fields from aliases in property_database.js

(just updating part 2 to remove the "prerequisites" field as well, now that it'll be auto-populated, as noted in comment 13)

Comment 16

[Daniel Holbert [:dholbert]]

Green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=54d02f56d9e2

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8669278 [details] [diff] [review]
part 1 v4: Make property_database.js automatically populate aliasing properties' *_values & prerequisites fields based on their alias target

Review of attachment 8669278 [details] [diff] [review]:
-----------------------------------------------------------------

I just realised we probably shouldn't be writing local variables like "prop", "entry', "aliasTargetEntry", etc. on to the global object, but there is a bit of that in property_database.js already.  Mind filing a bug to wrap bits of property_database.js into a (function() { ... })()?

Comment 18

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #17)
> I just realised we probably shouldn't be writing local variables like
> "prop", "entry', "aliasTargetEntry", etc. on to the global object, but there
> is a bit of that in property_database.js already.  Mind filing a bug to wrap
> bits of property_database.js into a (function() { ... })()?

(Using "let" might be an option, too, although I see at least one variable declared at the top level that we probably don't want to expose to tests, which "let" wouldn't help with according to my reading of https://groups.google.com/forum/#!searchin/mozilla.dev.platform/changes$20in$20chrome$20js$20code/mozilla.dev.platform/XoW35OxqDYI/hyBtAzaoAwAJ.)

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8669279 [details] [diff] [review]
part 2 v3: Remove now-unneeded *_values & prerequisites fields from aliases in property_database.js

Review of attachment 8669279 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't carefully check that the arrays you removed are identical to the now-inherited ones.

::: layout/style/test/property_database.js
@@ +6352,5 @@
>      invalid_values: [ "auto",  "1px" ]
>    };
>  }
>  
>  if (IsCSSPropertyPrefEnabled("layout.css.unset-value.enabled")) {

(At this point it's probably safe to remove this pref...)

Comment 20

[Daniel Holbert [:dholbert]]

Thanks for the review!

(In reply to Cameron McCormack (:heycam) from comment #17)
> Comment on attachment 8669278 [details] [diff] [review]
> part 1 v4: Make property_database.js automatically populate aliasing
> properties' *_values & prerequisites fields based on their alias target
> 
> I just realised we probably shouldn't be writing local variables like
> "prop", "entry', "aliasTargetEntry", etc. on to the global object, but there
> is a bit of that in property_database.js already.  Mind filing a bug to wrap
> bits of property_database.js into a (function() { ... })()?

Good point. I'm glad this hasn't (noticeably) broken anything yet. Filed bug 1211332.

(In reply to Cameron McCormack (:heycam) from comment #19)
> Comment on attachment 8669279 [details] [diff] [review]
> part 2 v3: Remove now-unneeded *_values & prerequisites fields from aliases
> in property_database.js
> 
> I didn't carefully check that the arrays you removed are identical to the
> now-inherited ones.

Nor did I yet, actually; I assumed that if there were any differences, it's pretty much guaranteed that it's because the aliases' arrays (the ones removed in this patch) have been neglected while the originals got updated.

Anyway, I'll do a sanity-check before landing to make sure that's the case.

> ::: layout/style/test/property_database.js
> >  if (IsCSSPropertyPrefEnabled("layout.css.unset-value.enabled")) {
> 
> (At this point it's probably safe to remove this pref...)

Filed bug 1211330.

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #20)
> > I didn't carefully check that the arrays you removed are identical to the
> > now-inherited ones.
> 
> Nor did I yet, actually; I assumed that if there were any differences, it's
> pretty much guaranteed that it's because the aliases' arrays (the ones
> removed in this patch) have been neglected while the originals got updated.
> 
> Anyway, I'll do a sanity-check before landing to make sure that's the case.

My assumption *almost* held up, with two exceptions:
 (1) -moz-{margin|padding}-{start|end} each have one "other_values" entry that's not included in their alias targets' "other_values" lists: 5%.  Their alias targets don't have any percentages in their other_values.

 (2) -moz-margin-{start|end} have an extensive invalid_values list, whereas their alias targets have only one invalid_values entry.

I'll clean both of these up (growing the remaining lists) before landing.

Comment 22

[Daniel Holbert [:dholbert]]

Try run w/ comment 21 addressed is looking good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=371bdc8063dd

Landed:
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/32e073eacabb
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1d65a48db29b

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/32e073eacabb
https://hg.mozilla.org/mozilla-central/rev/1d65a48db29b

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8669279 [details] [diff] [review]
part 2 v3: Remove now-unneeded *_values & prerequisites fields from aliases in property_database.js

>   "-moz-transform-style": {
>     domProp: "MozTransformStyle",
>     inherited: false,
>     type: CSS_TYPE_SHORTHAND_AND_LONGHAND,
>     alias_for: "transform-style",
>-    subproperties: [ "transform-style" ],
>-    initial_values: [ "flat" ],
>-    other_values: [ "preserve-3d" ],
>-    invalid_values: []
>   },

oops, this "subproperties" array-removal was accidental. Kudos to heycam for catching this in bug 1211101.

I filed bug 1212191 to catch errors of this sort in the future, and I pushed a one-liner to restore this removed line:
https://hg.mozilla.org/integration/mozilla-inbound/rev/530c5bc61a02

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/530c5bc61a02