Drop "aPresContext" arg from nsContainerFrame methods that can just call PresContext() (and from a few static nsContainerFrame methods that don't use it)

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla30
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

The nsContainerFrame superclass defines a few methods that take an unnecessary "aPresContext" argument.

(unnecessary because it's unused in some cases, and when it is used, it can just be replaced with a call to this->PresContext())

Filing this bug on dropping this arg.
This patch removes the nsPresContext argument from the following functions,  all declared in nsContainerFrame.h (using [this->]PresContext() instead, where it's even needed):

 nsContainerFrame::CreateNextInFlow
 nsContainerFrame::DeleteNextInFlowChild
 nsContainerFrame::ReparentFrameView (static method, doesn't use the arg)
 nsContainerFrame::ReparentFrameViewList  (static method, doesn't use the arg)
 nsContainerFrame::StealFrame
 nsContainerFrame::SetOverflowFrames
 nsContainerFrame::DestroyOverflowList
 nsContainerFrame::MoveOverflowToChildList
 nsContainerFrame::PushChildren
 nsContainerFrame::GetPropTableFrames
 nsContainerFrame::RemovePropTableFrames
 nsContainerFrame::SetPropTableFrames
 nsOverflowContinuationTracker constructor (doesn't use the arg)

...and this patch also updates the callers to no longer pass the now-removed arg (so that we can compile). (In many cases, this leaves us with an unused or only-used-once local variable, so I've also cleaned that up to replace such local variables with a single PresContext() invocation.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8370361 - Flags: review?(matspal)
A few notes, justifying the motivation for this bug:
 - In many cases, the callers are already directly passing "PresContext()" for this arg (when we could just be doing that internally)
 - While this patch may result in a few more PresContext() calls, these calls should be cheap. (i.e. their expense isn't enough to merit the complexity / annoyance of having to pass the arg everywhere) In particular, PresContext() is just a chain of 3 member-variable dereferences).

In particular, nsIFrame::PresContext is defined (inline in the header) as:
  return StyleContext()->RuleNode()->PresContext();
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIFrame.h#634
...and each of the methods it invokes (StyleContext(), RuleNode(), PresContext()) are defined in their own respective headers and just directly return a member-variable.
Here's a trivial whitespace-only followup to reindent the arguments in the affected function-signatures where aPresContext used to establish the indentation-amount. (by virtue of being the longest type)
Attachment #8370382 - Flags: review?(matspal)
OS: Linux → All
Hardware: x86_64 → All
Summary: Drop PresContext arg from nsContainerFrame methods → Drop "aPresContext" arg from nsContainerFrame methods that can just call PresContext() (and from a few static nsContainerFrame methods that don't use it)
Comment on attachment 8370361 [details] [diff] [review]
part 1: remove arg

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>+    nsContainerFrame::ReparentFrameViewList(aFrameList,
>                                             aOldParent, aNewParent);

Looks like it would fit on one line.

>+++ b/layout/generic/nsBlockFrame.cpp
>+    NS_ASSERTION(aPropValue ==
>+                 GetPropTableFrames(OverflowOutOfFlowsProperty()),
>                  "prop value mismatch");

The first arg fits on the same line?

>+++ b/layout/generic/nsCanvasFrame.cpp
>+      nsContainerFrame::ReparentFrameViewList(*overflow,
>                                               prevCanvasFrame, this);

one line? if not, move prevCanvasFrame to the first line.
(and FTR, I'd strongly prefer one line even if it's > 80 chars
over such unnatural breaking)

>+++ b/layout/generic/nsContainerFrame.cpp
>-  nsPresContext* pc = PresContext();
>   nsContainerFrame* lastParent = nullptr;
>   while (aOldFrame) {
>     //XXXfr probably should use StealFrame here. I'm not sure if we need to
>     //      check the overflow lists atm, but we'll need a prescontext lookup
>     //      for overflow containers once we can split abspos elements with
>     //      inline containing blocks.
>     nsIFrame* oldFrameNextContinuation = aOldFrame->GetNextContinuation();
>     nsContainerFrame* parent =
>       static_cast<nsContainerFrame*>(aOldFrame->GetParent());
>-    parent->StealFrame(pc, aOldFrame, true);
>+    parent->StealFrame(aOldFrame, true);
>     aOldFrame->Destroy();
>     aOldFrame = oldFrameNextContinuation;
>     if (parent != lastParent && generateReflowCommand) {
>-      pc->PresShell()->
>+      PresContext()->PresShell()->
>         FrameNeedsReflow(parent, nsIPresShell::eTreeChange,
>                          NS_FRAME_HAS_DIRTY_CHILDREN);
>       lastParent = parent;
>     }
>   }

In this case, I'd prefer the current code since it's a loop over
a continuation chain.  Even better, declare
"nsIPresShell* shell = PresContext()->PresShell()"
before the loop and use that?

>@@ -1075,62 +1071,59 @@ nsContainerFrame::ReflowOverflowContaine
>+        nsContainerFrame::ReparentFrameViewList(*excessFrames,
>                                                 prev, this);

Same line?

>+        SetPropTableFrames(overflowContainers,
>                            OverflowContainersProperty());

Same line?

>+      SetPropTableFrames(overflowContainers,
>                          OverflowContainersProperty());

Same line?

>+++ b/layout/generic/nsFirstLetterFrame.cpp
>@@ -348,17 +348,17 @@ nsFirstLetterFrame::DrainOverflowFrames(
>+      nsContainerFrame::ReparentFrameViewList(*overflowFrames,
>                                               prevInFlow, this);

Same line?

>+++ b/layout/generic/nsInlineFrame.cpp
>@@ -463,18 +462,17 @@ nsInlineFrame::PullOverflowsFromPrevInFl
>+      nsContainerFrame::ReparentFrameViewList(*prevOverflowFrames,
>                                               prevInFlow, this);

Same line?
Attachment #8370361 - Flags: review?(matspal) → review+
Comment on attachment 8370382 [details] [diff] [review]
part 2: reindent args in function signatures (whitespace-only)

>+++ b/layout/generic/nsBlockFrame.cpp
>+nsBlockFrame::StealFrame(nsIFrame* aChild,
>+                         bool      aForceNormal)

Looks like it fits on one line.

>+++ b/layout/generic/nsCanvasFrame.h
>+  virtual nsresult StealFrame(nsIFrame* aChild,
>+                              bool      aForceNormal) MOZ_OVERRIDE

ditto

>+++ b/layout/generic/nsColumnSetFrame.h
>+  virtual nsresult StealFrame(nsIFrame* aChild,
>+                              bool      aForceNormal) MOZ_OVERRIDE

ditto

>+++ b/layout/generic/nsContainerFrame.cpp
>+nsContainerFrame::StealFrame(nsIFrame* aChild,
>+                             bool      aForceNormal)

ditto

>+nsContainerFrame::PushChildren(nsIFrame* aFromChild,
>+                               nsIFrame* aPrevSibling)

ditto

>+++ b/layout/generic/nsContainerFrame.h
>+  nsresult CreateNextInFlow(nsIFrame*  aFrame,
>+                            nsIFrame*& aNextInFlowResult);

ditto

>+  virtual nsresult StealFrame(nsIFrame* aChild,
>+                              bool      aForceNormal = false);

Probably fits on one line too, but in this case I'd prefer
the separate line as you have it above to make the default arg
stand out.

>+  void PushChildren(nsIFrame* aFromChild,
>+                    nsIFrame* aPrevSibling);

Same line.
Attachment #8370382 - Flags: review?(matspal) → review+
And thanks for fixing this!
(In reply to Mats Palmgren (:mats) from comment #4)
I made the "same line" fixes for part 1 in my patch queue:
  https://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/rev/6ef92cd08bc8
(Kind of hard to read, since it's a diff-of-diff's, but the "++" lines are the new versions of the modified lines, of course.)

The chunks you mentioned in nsInlineFrame and nsFirstLetterFrame didn't quite fit on one line, so I just pulled up one of the arguments, per your comment about "if not, move prevCanvasFrame to the first line".
Addressed the "nsIPresShell* shell = PresContext()->PresShell()" before-the-loop review-comment here (good call on that, thanks!): https://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/rev/466b72088dff
(In reply to Mats Palmgren (:mats) from comment #5)
> >+++ b/layout/generic/nsBlockFrame.cpp
> >+nsBlockFrame::StealFrame(nsIFrame* aChild,
> >+                         bool      aForceNormal)
> 
> Looks like it fits on one line.

Personally, I actually prefer the separate-line-per-parameter style for function-signatures, in .cpp files at least, because it lets you easily skim over the function-signature and visually see the various parameter declarations, how many parameters there are, their order, etc.  Obviously this is less of an issue when there are just two parameters, but I think it still looks nice and is worthwhile from a consistency standpoint (and because it reduces the amount of reformatting required as more parameters are added).

The separate-line-per-parameter style also seems to be fairly common in gecko code (.cpp files at least), and my intent with this patch was not really to re-style a bunch of code -- just to re-beautify the indentation after the parameter-removal.

So I'd lean towards keeping the .cpp files as-they-stand in this patch.  needinfo'ing you to make sure you're OK with that. (If you'd strongly like these  function-signatures to be expressed as one-liners in the .cpp files, I'm still open to making that change.)

I did make the one-liner changes that you suggested for the .h files, though. (I'm a bit ambivalent about those as well, but I don't care as much there, since .h files are often more concise anyhow, so it makes more sense to collapse them there.)
 https://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/rev/8fc10e828f45
Flags: needinfo?(matspal)
I don't feel strongly about it, so leaving them as is in the .cpp file is fine.
Flags: needinfo?(matspal)
Cool, thanks (and thanks for the review!)

Try run: https://tbpl.mozilla.org/?tree=Try&rev=1aeaea0b8995
https://hg.mozilla.org/mozilla-central/rev/3072f0155515
https://hg.mozilla.org/mozilla-central/rev/810ff6004dcf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.