Closed Bug 1210905 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

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.
Attachment #8669094 - Flags: review?(cam)
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]
Attachment #8669094 - Attachment is obsolete: true
Attachment #8669094 - Flags: review?(cam)
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.)
Attachment #8669119 - Flags: review?(cam)
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
Attachment #8669121 - Flags: review?(cam)
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].
Attached patch (wrong patch file; ignore) (obsolete) — Splinter Review
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.)
Attachment #8669119 - Attachment is obsolete: true
Attachment #8669121 - Attachment is obsolete: true
Attachment #8669119 - Flags: review?(cam)
Attachment #8669121 - Flags: review?(cam)
Attachment #8669202 - Flags: review?(cam)
[sorry, that was a patch from a different bug; here's the correct updated patch]
Attachment #8669202 - Attachment is obsolete: true
Attachment #8669202 - Flags: review?(cam)
Attachment #8669203 - Flags: review?(cam)
Attachment #8669202 - Attachment description: part 1 v3: make aliases copy "*_values" fields from their alias-targets → (wrong patch file; ignore)
...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)
Attachment #8669204 - Flags: review?(cam)
Blocks: 1211101
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)
Attachment #8669203 - Attachment is obsolete: true
Attachment #8669203 - Flags: review?(cam)
Attachment #8669278 - Flags: review?(cam)
(just updating part 2 to remove the "prerequisites" field as well, now that it'll be auto-populated, as noted in comment 13)
Attachment #8669204 - Attachment is obsolete: true
Attachment #8669204 - Flags: review?(cam)
Attachment #8669279 - Flags: review?(cam)
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() { ... })()?
Attachment #8669278 - Flags: review?(cam) → review+
(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...)
Attachment #8669279 - Flags: review?(cam) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/32e073eacabb
https://hg.mozilla.org/mozilla-central/rev/1d65a48db29b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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
Blocks: 1216351
You need to log in before you can comment on or make changes to this bug.