841789 - rename nsIFrame::GetStyleContext to nsIFrame::StyleContext

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

In order to conform to our convention that methods that never return a null pointer shouldn't have Get in the name, I'll attach a patch to rename nsIFrame:GetStyleContext to StyleContext.  (There's more work in this area needed, but this is a start.)

Comment 1

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

Attachment: , patch 1: Rename nsIFrame::GetStyleContext() to nsIFrame::StyleContext() since it can never return null.

This makes it conform to our convention that getters returning pointers
that can never be null do not begin with "Get".

Comment 2

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

Attachment: , patch 2: Fix callers that are going through the style context to get style structs rather than using the nsIFrame shortcuts.

This is just a code simplification that I noticed while reading through
patch 1.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 714487 [details] [diff] [review]
, patch 2:  Fix callers that are going through the style context to get style structs rather than using the nsIFrame shortcuts.

>+++ b/layout/svg/nsSVGSwitchFrame.cpp
>@@ -108,17 +108,17 @@ NS_IMETHODIMP
> nsSVGSwitchFrame::PaintSVG(nsRenderingContext* aContext,
>                            const nsIntRect *aDirtyRect)
> {
>   NS_ASSERTION(!NS_SVGDisplayListPaintingEnabled() ||
>                (mState & NS_STATE_SVG_NONDISPLAY_CHILD),
>                "If display lists are enabled, only painting of non-display "
>                "SVG should take this code path");
> 
>-  const nsStyleDisplay *display = mStyleContext->GetStyleDisplay();
>+  const nsStyleDisplay *display = GetStyleDisplay();
>   if (display->mOpacity == 0.0)
>     return NS_OK;
> 
>   nsIFrame *kid = GetActiveChildFrame();
>   if (kid) {
>     nsSVGUtils::PaintFrameWithEffects(aContext, aDirtyRect, kid);
>   }
>   return NS_OK;

nit: looks like 'display' is only used once here, so we could drop that variable entirely move GetStyleDisplay() into the "if" check.

r=me either way

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 714486 [details] [diff] [review]
, patch 1: Rename nsIFrame::GetStyleContext() to nsIFrame::StyleContext() since it can never return null.

>@@ -10844,17 +10844,17 @@ nsCSSFrameConstructor::RemoveFloatingFir
[...]
>-  nsStyleContext* parentSC = parentFrame->GetStyleContext();
>+  nsStyleContext* parentSC = parentFrame->StyleContext();
>   if (!parentSC) {
>     return NS_OK;
>   }

The null-check on parentSC is now unnecessary, by definition.  Probably worth fixing in a followup patch (or followup bug).

>diff --git a/layout/generic/nsFirstLetterFrame.cpp b/layout/generic/nsFirstLetterFrame.cpp
>-  nsStyleContext* parentSC = this->GetStyleContext()->GetParent();
>+  nsStyleContext* parentSC = this->StyleContext()->GetParent();
>   if (parentSC) {

Here, too.

>+++ b/layout/generic/nsInlineFrame.cpp
>@@ -1050,17 +1050,17 @@ nsFirstLineFrame::Reflow(nsPresContext* 
>-      nsStyleContext* parentContext = first->GetParent()->GetStyleContext();
>+      nsStyleContext* parentContext = first->GetParent()->StyleContext();
>       if (parentContext) {

Here, too.

>+++ b/layout/generic/nsIFrame.h
>   /**
>    * Get the style context associated with this frame.
>    *
>    */
>-  nsStyleContext* GetStyleContext() const { return mStyleContext; }
>+  nsStyleContext* StyleContext() const { return mStyleContext; }

Might be worth dropping that bonus blank line from the javadoc-comment, as long as you're touching this.

r=me regardless, though.

Comment 5

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

(In reply to Daniel Holbert [:dholbert] from comment #4)
> >diff --git a/layout/generic/nsFirstLetterFrame.cpp b/layout/generic/nsFirstLetterFrame.cpp
> >-  nsStyleContext* parentSC = this->GetStyleContext()->GetParent();
> >+  nsStyleContext* parentSC = this->StyleContext()->GetParent();
> >   if (parentSC) {
> 
> Here, too.

This one is different because of the ->GetParent() on the end.

But I'll fix the rest in a patch 3 (so that patch 1 is just the rename).

Comment 6

[Daniel Holbert [:dholbert]]

Oh, right. Thanks!

Comment 7

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

   https://hg.mozilla.org/integration/mozilla-inbound/rev/095bd7117b4e
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f5714b93d6ca
   https://hg.mozilla.org/integration/mozilla-inbound/rev/25756f81ccbf

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/095bd7117b4e
https://hg.mozilla.org/mozilla-central/rev/f5714b93d6ca
https://hg.mozilla.org/mozilla-central/rev/25756f81ccbf