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

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

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

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

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22) > trailing whitepsace Fixed. Thanks for the quick review.

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