476598 - rename inherited/aInherited in nsRuleNode

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The meaning of variables called inherited/aInherited in nsRuleNode.cpp is very confusing; I think it's one of the common sources of mistakes by new contributors to style code.

I think we should rename these variables to something more like what they do.

I think the easiest option is to rename them to canStoreInRuleTree / aCanStoreInRuleTree, and flipping the boolean values.

While there, we should either:
 (1) change the aInherited values that aren't mutated (in the functions that have both aInherited and inherited) to be |const| parameters, or
 (2) get rid of that duplication and mutate the arguments.

Thoughts?

Comment 1

[Boris Zbarsky [:bzbarsky]]

That sounds good to me.  Another option is to rename to dependsOnParent and aDependsOnParent and not flip, but the canStore naming is somewhat clearer as to why we really care about them.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: fix two uses of aInherited that should have been inherited

This is a needed for patch 2 to compile, but also fixes a real bug, and should land on 1.9.1.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: make the aInheriteds that shouldn't be modified |const|

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: reverse the true/false sense of aInherited

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: rename aInherited to aCanStoreInRuleTree

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 360433 [details] [diff] [review]
patch 1: fix two uses of aInherited that should have been inherited

Want to write regression tests for this too?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: fix two uses of aInherited that should have been inherited

With tests.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 360556 [details] [diff] [review]
patch 1: fix two uses of aInherited that should have been inherited

This fixes a pretty bad bug in -moz-transform-origin (a new 1.9.1 feature) that was caught by the cleanup in this bug.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/af13f36fb9d8
http://hg.mozilla.org/mozilla-central/rev/6f337cbf2aed
http://hg.mozilla.org/mozilla-central/rev/db15edcc02ce
http://hg.mozilla.org/mozilla-central/rev/d66a096fddd6

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2d248b150ce6

Adding the fixed1.9.1 keyword since the part that's important to fix for 1.9.1 is.