Closed Bug 781360 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(5 files)

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.
Assignee: nobody → dbaron
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)
Attachment #714749 - Flags: review?(dholbert) → review+
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.
Attachment #714750 - Flags: review?(dholbert) → review+
Patches 3a and 3b are needed for each other to compile, and are intended
to be squashed together post-review.
Attachment #714765 - Flags: review?(dholbert)
Patches 3a and 3b are needed for each other to compile, and are intended
to be squashed together post-review.
Attachment #714766 - Flags: review?(dholbert)
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
(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 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.
Attachment #714765 - Flags: review?(dholbert) → review+
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
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.