508665 - make mParent an nsContainerFrame*

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[fantasai]

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. :)

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1, Change the signature of SetParent/GetParent from nsIFrame* to nsContainerFrame*

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2, Move nsCSSFrameConstructor::GetFrameFor to a static function in nsCSSFrameConstructor.cpp

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 3b, Deal with GetContentInsertionFrame() returning null in a couple of places

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 3, Change GetContentInsertionFrame() to return a nsContainerFrame*, and return null for leaf frames

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 4, Make nsCSSFrameConstructor use nsContainerFrame* for frames used as parent frames

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 5, Make nsIFrame::Init require a nsContainerFrame* for the parent frame param

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 6, Require a nsContainerFrame* for aParent in nsFrameList methods

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 7, Require a nsContainerFrame* for aParent in nsFrameManager methods

Comment 9

[Mats Palmgren (inactive)]

Attachment: part 8, Move child frame insertion/removal methods from nsIFrame to nsContainerFrame

Comment 10

[Mats Palmgren (inactive)]

Attachment: part 9, Remove now redundant static_cast<nsContainerFrame*> and do_QueryFrame() calls

Comment 11

[Mats Palmgren (inactive)]

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

Comment 12

[Mats Palmgren (inactive)]

Attachment: part 11, Make nsIFrame::SetParent non-virtual

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 12, Move nsIFrame::GetChildBox/GetNextBox/GetParentBox back to XUL

Comment 14

[Mats Palmgren (inactive)]

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

Comment 15

[Mats Palmgren (inactive)]

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

Comment 16

[Mats Palmgren (inactive)]

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

Comment 17

[Mats Palmgren (inactive)]

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

Comment 18

[Mats Palmgren (inactive)]

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.

Comment 19

[Mats Palmgren (inactive)]

Attachment: wdiff of one of the hunks in part 4.

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).

Comment 20

[Mats Palmgren (inactive)]

The only functional change of significance is that GetContentInsertionFrame
now returns null for leaf frames (it used to return the frame itself).

Comment 21

[Mats Palmgren (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=d9a068370835
https://tbpl.mozilla.org/?tree=Try&rev=21ca8ab99904

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 23

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88e93734e132
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d60a59865d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/75c14b62556e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa36dab2765
https://hg.mozilla.org/integration/mozilla-inbound/rev/18214a2cfdb3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a86816c83941
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c8a275251b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/c22a627d243b
https://hg.mozilla.org/integration/mozilla-inbound/rev/26134643a8c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/0634d0a57ae5
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b2dee6f0b78
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0a6a02b6f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/8620fa86ccef
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc3b4b1ac883
https://hg.mozilla.org/integration/mozilla-inbound/rev/44a39c09abb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/db861719b5d0

Comment 24

[Mats Palmgren (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> trailing whitepsace

Fixed.  Thanks for the quick review.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/88e93734e132
https://hg.mozilla.org/mozilla-central/rev/1d60a59865d0
https://hg.mozilla.org/mozilla-central/rev/75c14b62556e
https://hg.mozilla.org/mozilla-central/rev/8aa36dab2765
https://hg.mozilla.org/mozilla-central/rev/18214a2cfdb3
https://hg.mozilla.org/mozilla-central/rev/a86816c83941
https://hg.mozilla.org/mozilla-central/rev/6c8a275251b6
https://hg.mozilla.org/mozilla-central/rev/c22a627d243b
https://hg.mozilla.org/mozilla-central/rev/26134643a8c5
https://hg.mozilla.org/mozilla-central/rev/0634d0a57ae5
https://hg.mozilla.org/mozilla-central/rev/2b2dee6f0b78
https://hg.mozilla.org/mozilla-central/rev/7b0a6a02b6f4
https://hg.mozilla.org/mozilla-central/rev/8620fa86ccef
https://hg.mozilla.org/mozilla-central/rev/fc3b4b1ac883
https://hg.mozilla.org/mozilla-central/rev/44a39c09abb8
https://hg.mozilla.org/mozilla-central/rev/db861719b5d0