fantasai Reporter
	Description • 15 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 18 • 10 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 19 • 10 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 20 • 10 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 21 • 10 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 22 • 10 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 23 • 10 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 24 • 10 years ago

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

Fixed.  Thanks for the quick review.

	Carsten Book [:Tomcat]
	Comment 25 • 10 years ago

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