Closed Bug 508665 Opened 10 years ago Closed 6 years ago

make mParent an nsContainerFrame*

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: fantasai.bugs, Assigned: mats)

References

Details

Attachments

(18 files)

44.42 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.96 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.00 KB, patch
roc
: review+
Details | Diff | Splinter Review
55.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
236.07 KB, patch
roc
: review+
Details | Diff | Splinter Review
129.82 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.07 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.22 KB, patch
roc
: review+
Details | Diff | Splinter Review
25.97 KB, patch
roc
: review+
Details | Diff | Splinter Review
18.16 KB, patch
roc
: review+
Details | Diff | Splinter Review
19.68 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
49.28 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.00 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.83 KB, patch
roc
: review+
Details | Diff | Splinter Review
29.47 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.71 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.93 KB, patch
Details | Diff | Splinter Review
Casting mParent to nsContainerFrame all the time makes me a bit uncomfortable, because I keep wondering if weird parts of our code might be using something else. So I'd like to make mParent an nsContainerFrame*.

bz says GetParent() needs to return an nsIFrame* for external callers. He says we could maybe do this with something like

  virtual nsIFrame* GetParentExternal(); /* implemented in nsFrame.cpp */
#ifdef _IMPL_NS_LAYOUT
  nsContainerFrame* GetParent() { return mParent; }
#else
  nsIFrame* GetParent() { return GetParentExternal(); }
#endif

I'm ok with doing that, or keep using static_cast<nsContainerFrame*>GetParent() (at least within layout I know it's an nsContainerFrame* because SetParent() can complain if it's not), or using mParent directly.

I'll attach a patch with bz's suggestion and people can complain or not as appropriate. :)
Assignee: nobody → matspal
Status: NEW → ASSIGNED
Attachment #8428007 - Flags: review?(roc)
Attachment #8428009 - Attachment description: part 3, Change GetContentInsertionFrame() to return a nsContainerFrame*, and return null for leaf frames → part 3b, Deal with GetContentInsertionFrame() returning null in a couple of places
Attachment #8428015 - Attachment description: Make nsIFrame::Init require a nsContainerFrame* for the parent frame param → part 5, Make nsIFrame::Init require a nsContainerFrame* for the parent frame param
Attachment #8428018 - Attachment description: Require a nsContainerFrame* for aParent in nsFrameManager methods → part 7, Require a nsContainerFrame* for aParent in nsFrameManager methods
Summary of the patch series:

Part 1, method signature changes (nsIFrame):
-  virtual void SetParent(nsIFrame* aParent) = 0;
+  virtual void SetParent(nsContainerFrame* aParent) = 0;
-  nsIFrame* GetParent() const
+  nsContainerFrame* GetParent() const

Part 2, move nsCSSFrameConstructor::GetFrameFor to a static function
in nsCSSFrameConstructor.cpp

Part 3, method signature change (nsIFrame):
-  virtual nsIFrame* GetContentInsertionFrame()
+  virtual nsContainerFrame* GetContentInsertionFrame()

Part 3b, fix a couple of places that assumed GetContentInsertionFrame
can't return null

Part 4,  Make nsCSSFrameConstructor use nsContainerFrame* for frames
used as parent frames.

Part 5, signature change (nsIFrame):
-  virtual void Init(nsIContent*      aContent,
-                    nsIFrame*        aParent,
-                    nsIFrame*        aPrevInFlow) = 0;
+  virtual void Init(nsIContent*       aContent,
+                    nsContainerFrame* aParent,
+                    nsIFrame*         aPrevInFlow) = 0;

Part 6, signature changes in nsFrameList methods:
-                    nsIFrame*        aParent,
+                    nsContainerFrame* aParent,

Part 7, signature changes in nsFrameManager methods:
-                    nsIFrame*        aParent,
+                    nsContainerFrame* aParent,

Part 8, move SetInitialChildList,AppendFrames,InsertFrames and
RemoveFrame from nsIFrame to nsContainerFrame.

part 9, remove now redundant static_cast<nsContainerFrame*> and
do_QueryFrame() calls.

part 10, Replace nsMenuFrame::mMenuParent with a GetMenuParent()
method to eliminate the need for a SetParent override.

part 11, make nsIFrame::SetParent non-virtual

part 12, move nsIFrame::GetChildBox/GetNextBox/GetParentBox to XUL

part 13, move nsIFrame::IsBoxWrapped to a static function in nsFrame.cpp

part 14, uninline nsIFrame::GetPositionIgnoringScrolling() (since it
uses a nsContainerFrame method)

part 15, s/mParent/GetParent()/ in a bunch of nsIFrame sub-classes

part 16, Change the type of nsIFrame::mParent to nsContainerFrame* and
make it private.

==============

All parts builds independently of later parts.
All parts, except part 3, should also work independently of later parts.
I'll fold part 3b into 3 before landing so that all changesets results
in a working build.
There is one change in part 4 that I want to highlight since it's
non-trivial compared with the rest.  This attachment is a wdiff
of the particular hunk in ConstructFrameFromItemInternal starting here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=d4ec70824b69#3695

I'm changing the code so that the block starting from line 3702
is only run if the newly created frame is a nsContainerFrame, up
until line 3813.  That part only applies to frames that can have
child frames anyway, so it should be a NOP for leaf frames.
Except one block inside it - the "More icky XUL stuff" on line 3766
which should run for all frames - so I moved that out and do it
after said nsContainerFrame-only block.

Let me know if you have questions (on any of the patches).
The only functional change of significance is that GetContentInsertionFrame
now returns null for leaf frames (it used to return the frame itself).
Comment on attachment 8428013 [details] [diff] [review]
part 3, Change GetContentInsertionFrame() to return a nsContainerFrame*, and return null for leaf frames

Review of attachment 8428013 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCaret.cpp
@@ +305,5 @@
> +  if (!frame) {
> +    frame = aFrame;
> +  }
> +  NS_ASSERTION(!(frame->GetStateBits() & NS_FRAME_IN_REFLOW),
> +               "We should not be in the middle of reflow");  

trailing whitepsace
Attachment #8428013 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> trailing whitepsace

Fixed.  Thanks for the quick review.
Depends on: 1061028
Depends on: 1116104
You need to log in before you can comment on or make changes to this bug.