781360 - make style system getters that can't return null not have Get in the name

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

We have a coding convention that getters that can never return null shouldn't have Get at the beginning of the name.  However, there are a bunch of widely-used getters in the style system that have never been updated since we established that convention.  In particular, I'm thinking of:

  nsIFrame::GetStyleContext()
  nsStyleContext::GetRuleNode
  nsRuleNode::GetPresContext
  nsIFrame::GetStyle*  (macro-generated from nsStyleStructList.h)
  nsStyleContext::GetStyle*  (macro-generated from nsStyleStructList.h)

We should rename these methods to remove the initial "Get" from the name.

Comment 1

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Attachment: patch 1: Rename nsStyleContext::GetRuleNode to RuleNode, since it can never return null.

Comment 2

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Attachment: patch 2: Rename nsRuleNode::GetPresContext to PresContext, since it can never return null.

Comment 3

[Daniel Holbert [:dholbert] (blocking needinfo/review, due to AFK on parental leave 12/9 - 1/6)]

Comment on attachment 714749 [details] [diff] [review]
patch 1:  Rename nsStyleContext::GetRuleNode to RuleNode, since it can never return null.

r=me

It's probably worth adding an assertion to the nsStyleContext constructor, to assert that mRuleNode is non-null there.  (i.e. the constructor's not being invoked with a null rule-node pointer)  That assumption plus the const-ness of mRuleNode are what guarantee that RuleNode() can never return null.

(We'd crash pretty much immediately if the constructor were invoked with a null rule node, of course -- so this assertion would be more of a documentation &  making-our-assumptions-explicit sort of thing, rather than a bug-catching thing)

Comment 4

[Daniel Holbert [:dholbert] (blocking needinfo/review, due to AFK on parental leave 12/9 - 1/6)]

Comment on attachment 714750 [details] [diff] [review]
patch 2:  Rename nsRuleNode::GetPresContext to PresContext, since it can never return null.

It'd be worth adding a similar assertion to the one I suggested above (that mPresContext is non-null in the constructor), and also making mPresContext a const pointer (can't be changed to point to something else (especially not null)) in the nsRuleNode class definition.

r=me with that.

Comment 5

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Attachment: patch 3a: Rename {nsIFrame,nsStyleContext,nsComputedDOMStyle}::GetStyle* to Style*, since they can never return null. Automated changes only.

Patches 3a and 3b are needed for each other to compile, and are intended
to be squashed together post-review.

Comment 6

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Attachment: patch 3b: Rename {nsIFrame,nsStyleContext,nsComputedDOMStyle}::GetStyle* to Style*, since they can never return null.

Patches 3a and 3b are needed for each other to compile, and are intended
to be squashed together post-review.

Comment 7

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Attachment: patch 4: Remove the Get from nsStyleContext::GetStyleData and nsIFrame::GetStyleDataExternal, which never return null.

Comment 8

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

To be extra clear, patch 3a was written by the follwing sed script:

s/\<GetStyle\(Font\|Color\|List\|Text\|Visibility\|Quotes\|UserInterface\|TableBorder\|SVG\|Background\|Position\|TextReset\|Display\|Content\|UIReset\|Table\|Margin\|Padding\|Border\|Outline\|XUL\|SVGReset\|Column\)\>/Style\1/g

Comment 9

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> It'd be worth adding a similar assertion to the one I suggested above (that
> mPresContext is non-null in the constructor), and also making mPresContext a
> const pointer (can't be changed to point to something else (especially not
> null)) in the nsRuleNode class definition.

While making mPresContext const, I'll do the same for mRule and mParent, since they're likewise immutable.

Comment 10

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Landed patches 1 and 2:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dd7f52016f53
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9522b45f9ef9

Comment 11

[Daniel Holbert [:dholbert] (blocking needinfo/review, due to AFK on parental leave 12/9 - 1/6)]

Comment on attachment 714765 [details] [diff] [review]
patch 3a: Rename {nsIFrame,nsStyleContext,nsComputedDOMStyle}::GetStyle* to Style*, since they can never return null.  Automated changes only.

comment 8's sed command looks good to me.

Rather than reading the 700k patch, I ran the command myself to generate a patch, and compared that against the attached patch.  I only noticed only one relevant difference -- nsRangeFrame.cpp was added in the time since you generated the attached patch, so that will need the script applied (for a GetStyleFont() call).

Looks like there's also some other less-important bitrot as well, largely caused by the infallible-display-list stuff that just landed in bug 840902.  I imagine you'll want to just re-run the script before landing, rather than hand-fixing nsRangeFrame and the bitrot.

Anyway,  r=me (with the nsRangeFrame fix), as long as this compiles.

Comment 12

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Yep, I'd rerun the sed script this morning with the same results.

Landed patches 3 and 4:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/343315251a40
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/81fac90f0e9f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dd7f52016f53
https://hg.mozilla.org/mozilla-central/rev/9522b45f9ef9
https://hg.mozilla.org/mozilla-central/rev/343315251a40
https://hg.mozilla.org/mozilla-central/rev/81fac90f0e9f