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 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]

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

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

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.

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

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

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

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

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

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

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

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.

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

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

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

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() { ... })()?

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

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.

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

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

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

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