From c3fb633256002981adccc6a457c88a69a51762f1 Mon Sep 17 00:00:00 2001 From: Mats Palmgren Date: Fri, 18 Sep 2009 13:09:36 +0200 Subject: [PATCH] Bug 233463, patch 3 - Make Destroy/RemoveFrame() methods void and assert that the frame to remove is present. r=bzbarsky --- layout/base/nsCSSFrameConstructor.cpp | 7 +--- layout/generic/nsAbsoluteContainingBlock.cpp | 7 +--- layout/generic/nsAbsoluteContainingBlock.h | 6 +-- layout/generic/nsBlockFrame.cpp | 13 ++++--- layout/generic/nsContainerFrame.cpp | 19 +++++----- layout/generic/nsFrameList.cpp | 32 +++++++++++++--- layout/generic/nsFrameList.h | 39 +++++++++++++------- layout/generic/nsHTMLFrame.cpp | 6 ++- layout/generic/nsInlineFrame.cpp | 3 +- layout/generic/nsViewportFrame.cpp | 3 +- layout/svg/base/src/nsSVGContainerFrame.cpp | 3 +- layout/tables/nsTableColGroupFrame.cpp | 21 +++++------ 12 files changed, 94 insertions(+), 65 deletions(-) diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp index 587e55ff6825..ff04fde360dd 100644 --- a/layout/base/nsCSSFrameConstructor.cpp +++ b/layout/base/nsCSSFrameConstructor.cpp @@ -1340,11 +1340,8 @@ AdjustFloatParentPtrs(nsIFrame* aFrame, NS_ASSERTION(outOfFlowFrame->GetParent() == aOuterState.mFloatedItems.containingBlock, "expected the float to be a child of the outer CB"); - if (aOuterState.mFloatedItems.RemoveFrame(outOfFlowFrame, nsnull)) { - aState.mFloatedItems.AddChild(outOfFlowFrame); - } else { - NS_NOTREACHED("float wasn't in the outer state float list"); - } + aOuterState.mFloatedItems.RemoveFrame(outOfFlowFrame); + aState.mFloatedItems.AddChild(outOfFlowFrame); outOfFlowFrame->SetParent(parent); if (outOfFlowFrame->GetStateBits() & diff --git a/layout/generic/nsAbsoluteContainingBlock.cpp b/layout/generic/nsAbsoluteContainingBlock.cpp index dba3fefc6ac9..a1f52fd7e919 100644 --- a/layout/generic/nsAbsoluteContainingBlock.cpp +++ b/layout/generic/nsAbsoluteContainingBlock.cpp @@ -109,7 +109,7 @@ nsAbsoluteContainingBlock::InsertFrames(nsIFrame* aDelegatingFrame, NS_FRAME_HAS_DIRTY_CHILDREN); } -nsresult +void nsAbsoluteContainingBlock::RemoveFrame(nsIFrame* aDelegatingFrame, nsIAtom* aListName, nsIFrame* aOldFrame) @@ -121,10 +121,7 @@ nsAbsoluteContainingBlock::RemoveFrame(nsIFrame* aDelegatingFrame, ->DeleteNextInFlowChild(aOldFrame->PresContext(), nif, PR_FALSE); } - PRBool result = mAbsoluteFrames.DestroyFrame(aOldFrame); - NS_ASSERTION(result, "didn't find frame to delete"); - - return result ? NS_OK : NS_ERROR_FAILURE; + mAbsoluteFrames.DestroyFrame(aOldFrame); } nsresult diff --git a/layout/generic/nsAbsoluteContainingBlock.h b/layout/generic/nsAbsoluteContainingBlock.h index 5ce6a23f728d..511c2079da30 100644 --- a/layout/generic/nsAbsoluteContainingBlock.h +++ b/layout/generic/nsAbsoluteContainingBlock.h @@ -96,9 +96,9 @@ public: nsIAtom* aListName, nsIFrame* aPrevFrame, nsFrameList& aFrameList); - nsresult RemoveFrame(nsIFrame* aDelegatingFrame, - nsIAtom* aListName, - nsIFrame* aOldFrame); + void RemoveFrame(nsIFrame* aDelegatingFrame, + nsIAtom* aListName, + nsIFrame* aOldFrame); // Called by the delegating frame after it has done its reflow first. This // function will reflow any absolutely positioned child frames that need to diff --git a/layout/generic/nsBlockFrame.cpp b/layout/generic/nsBlockFrame.cpp index 9a02d71424ae..cb681e0ba3c6 100644 --- a/layout/generic/nsBlockFrame.cpp +++ b/layout/generic/nsBlockFrame.cpp @@ -4809,14 +4809,14 @@ nsBlockFrame::RemoveFloat(nsIFrame* aFloat) { } // Try to destroy if it's in mFloats. - if (mFloats.DestroyFrame(aFloat)) { + if (mFloats.DestroyFrameIfPresent(aFloat)) { return line; } // Try our overflow list { nsAutoOOFFrameList oofs(this); - if (oofs.mList.DestroyFrame(aFloat)) { + if (oofs.mList.DestroyFrameIfPresent(aFloat)) { return line_end; } } @@ -4885,7 +4885,8 @@ nsBlockFrame::RemoveFrame(nsIAtom* aListName, } } else if (nsGkAtoms::absoluteList == aListName) { - return mAbsoluteContainer.RemoveFrame(this, aListName, aOldFrame); + mAbsoluteContainer.RemoveFrame(this, aListName, aOldFrame); + return NS_OK; } else if (nsGkAtoms::floatList == aListName) { // Make sure to mark affected lines dirty for the float frame @@ -5378,15 +5379,15 @@ nsBlockFrame::StealFrame(nsPresContext* aPresContext, if ((aChild->GetStateBits() & NS_FRAME_OUT_OF_FLOW) && aChild->GetStyleDisplay()->IsFloating()) { - PRBool removed = mFloats.RemoveFrame(aChild); + PRBool removed = mFloats.RemoveFrameIfPresent(aChild); if (!removed) { nsFrameList* list = GetPropTableFrames(aPresContext, nsGkAtoms::floatContinuationProperty); if (list) { - removed = list->RemoveFrame(aChild); + removed = list->RemoveFrameIfPresent(aChild); } } - return (removed) ? NS_OK : NS_ERROR_UNEXPECTED; + return removed ? NS_OK : NS_ERROR_UNEXPECTED; } if ((aChild->GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER) diff --git a/layout/generic/nsContainerFrame.cpp b/layout/generic/nsContainerFrame.cpp index f99273fb0488..10d33bb01847 100644 --- a/layout/generic/nsContainerFrame.cpp +++ b/layout/generic/nsContainerFrame.cpp @@ -228,14 +228,10 @@ nsContainerFrame::RemoveFrame(nsIAtom* aListName, // for overflow containers once we can split abspos elements with // inline containing blocks. if (parent == this) { - if (!parent->mFrames.DestroyFrame(aOldFrame)) { + if (!parent->mFrames.DestroyFrameIfPresent(aOldFrame)) { // Try to remove it from our overflow list, if we have one. // The simplest way is to reuse StealFrame. -#ifdef DEBUG - nsresult rv = -#endif - StealFrame(PresContext(), aOldFrame, PR_TRUE); - NS_ASSERTION(NS_SUCCEEDED(rv), "Could not find frame to remove!"); + StealFrame(PresContext(), aOldFrame, PR_TRUE); aOldFrame->Destroy(); } } else { @@ -1084,19 +1080,22 @@ nsContainerFrame::StealFrame(nsPresContext* aPresContext, } } else { - if (!mFrames.RemoveFrame(aChild)) { + if (!mFrames.RemoveFrameIfPresent(aChild)) { + removed = PR_FALSE; // We didn't find the child in the parent's principal child list. // Maybe it's on the overflow list? nsFrameList* frameList = GetOverflowFrames(); if (frameList) { - removed = frameList->RemoveFrame(aChild); + removed = frameList->RemoveFrameIfPresent(aChild); if (frameList->IsEmpty()) { DestroyOverflowList(aPresContext); } } } } - return (removed) ? NS_OK : NS_ERROR_UNEXPECTED; + + NS_POSTCONDITION(removed, "StealFrame: can't find aChild"); + return removed ? NS_OK : NS_ERROR_UNEXPECTED; } nsFrameList @@ -1261,7 +1260,7 @@ nsContainerFrame::RemovePropTableFrame(nsPresContext* aPresContext, // No such list return PR_FALSE; } - if (!frameList->RemoveFrame(aFrame)) { + if (!frameList->RemoveFrameIfPresent(aFrame)) { // Found list, but it doesn't have the frame. Put list back. SetPropTableFrames(aPresContext, frameList, aPropID); return PR_FALSE; diff --git a/layout/generic/nsFrameList.cpp b/layout/generic/nsFrameList.cpp index 1cd6c30b033c..c85f277d6cb8 100644 --- a/layout/generic/nsFrameList.cpp +++ b/layout/generic/nsFrameList.cpp @@ -100,10 +100,11 @@ nsFrameList::SetFrames(nsIFrame* aFrameList) mLastChild = nsLayoutUtils::GetLastSibling(mFirstChild); } -PRBool +void nsFrameList::RemoveFrame(nsIFrame* aFrame, nsIFrame* aPrevSiblingHint) { NS_PRECONDITION(aFrame, "null ptr"); + NS_PRECONDITION(ContainsFrame(aFrame), "wrong list"); nsIFrame* nextFrame = aFrame->GetNextSibling(); if (aFrame == mFirstChild) { @@ -112,7 +113,6 @@ nsFrameList::RemoveFrame(nsIFrame* aFrame, nsIFrame* aPrevSiblingHint) if (!nextFrame) { mLastChild = nsnull; } - return PR_TRUE; } else { nsIFrame* prevSibling = aPrevSiblingHint; @@ -125,10 +125,21 @@ nsFrameList::RemoveFrame(nsIFrame* aFrame, nsIFrame* aPrevSiblingHint) if (!nextFrame) { mLastChild = prevSibling; } + } + } +} + +PRBool +nsFrameList::RemoveFrameIfPresent(nsIFrame* aFrame) +{ + NS_PRECONDITION(aFrame, "null ptr"); + + for (FrameLinkEnumerator iter(*this); !iter.AtEnd(); iter.Next()) { + if (iter.NextFrame() == aFrame) { + RemoveFrame(aFrame, iter.PrevFrame()); return PR_TRUE; } } - return PR_FALSE; } @@ -156,11 +167,20 @@ nsFrameList::RemoveFirstChild() return PR_FALSE; } -PRBool +void nsFrameList::DestroyFrame(nsIFrame* aFrame, nsIFrame* aPrevSiblingHint) { - NS_PRECONDITION(nsnull != aFrame, "null ptr"); - if (RemoveFrame(aFrame, aPrevSiblingHint)) { + NS_PRECONDITION(aFrame, "null ptr"); + RemoveFrame(aFrame, aPrevSiblingHint); + aFrame->Destroy(); +} + +PRBool +nsFrameList::DestroyFrameIfPresent(nsIFrame* aFrame) +{ + NS_PRECONDITION(aFrame, "null ptr"); + + if (RemoveFrameIfPresent(aFrame)) { aFrame->Destroy(); return PR_TRUE; } diff --git a/layout/generic/nsFrameList.h b/layout/generic/nsFrameList.h index 1ac5a54e0816..e8f1f4859c0f 100644 --- a/layout/generic/nsFrameList.h +++ b/layout/generic/nsFrameList.h @@ -134,14 +134,20 @@ public: } /** - * Take aFrame out of the frame list. This also disconnects aFrame - * from the sibling list. This will return PR_FALSE if aFrame is - * nsnull or if aFrame is not in the list. The second frame is - * a hint for the prev-sibling of aFrame; if the hint is correct, - * then this is O(1) time. If successfully removed, the child's - * NextSibling pointer is cleared. + * from the sibling list. The frame must be non-null and present on + * this list. + * The second frame is a hint for the prev-sibling of aFrame; if the + * hint is correct, then this is O(1) time. */ - PRBool RemoveFrame(nsIFrame* aFrame, nsIFrame* aPrevSiblingHint = nsnull); + void RemoveFrame(nsIFrame* aFrame, nsIFrame* aPrevSiblingHint = nsnull); + + /** + * Take aFrame out of the frame list, if present. This also disconnects + * aFrame from the sibling list. aFrame must be non-null but is not + * required to be on the list. + * @return PR_TRUE if aFrame was removed + */ + PRBool RemoveFrameIfPresent(nsIFrame* aFrame); /** * Take the frames after aAfterFrame out of the frame list. @@ -157,14 +163,19 @@ public: PRBool RemoveFirstChild(); /** - * Take aFrame out of the frame list and then destroy it. This also - * disconnects aFrame from the sibling list. This will return - * PR_FALSE if aFrame is nsnull or if aFrame is not in the list. The - * second frame is a hint for the prev-sibling of aFrame; if the - * hint is correct, then the time this method takes doesn't depend - * on the number of previous siblings of aFrame. + * Take aFrame out of the frame list and then destroy it. + * The frame must be non-null and present on this list. + * The second frame is a hint for the prev-sibling of aFrame; if the + * hint is correct, then this is O(1) time. */ - PRBool DestroyFrame(nsIFrame* aFrame, nsIFrame* aPrevSiblingHint = nsnull); + void DestroyFrame(nsIFrame* aFrame, nsIFrame* aPrevSiblingHint = nsnull); + + /** + * If aFrame is present on this list then take it out of the list and + * then destroy it. The frame must be non-null. + * @return PR_TRUE if the frame was found + */ + PRBool DestroyFrameIfPresent(nsIFrame* aFrame); /** * Insert aFrame right after aPrevSibling, or prepend it to this diff --git a/layout/generic/nsHTMLFrame.cpp b/layout/generic/nsHTMLFrame.cpp index 66106d65bd50..7247cd23db71 100644 --- a/layout/generic/nsHTMLFrame.cpp +++ b/layout/generic/nsHTMLFrame.cpp @@ -352,8 +352,10 @@ CanvasFrame::RemoveFrame(nsIAtom* aListName, { nsresult rv; - if (nsGkAtoms::absoluteList == aListName) - return mAbsoluteContainer.RemoveFrame(this, aListName, aOldFrame); + if (nsGkAtoms::absoluteList == aListName) { + mAbsoluteContainer.RemoveFrame(this, aListName, aOldFrame); + return NS_OK; + } NS_ASSERTION(!aListName, "unexpected child list name"); if (aListName) { diff --git a/layout/generic/nsInlineFrame.cpp b/layout/generic/nsInlineFrame.cpp index 2f6ddbe458cd..20fa5c3afb22 100644 --- a/layout/generic/nsInlineFrame.cpp +++ b/layout/generic/nsInlineFrame.cpp @@ -1133,7 +1133,8 @@ nsPositionedInlineFrame::RemoveFrame(nsIAtom* aListName, nsresult rv; if (nsGkAtoms::absoluteList == aListName) { - rv = mAbsoluteContainer.RemoveFrame(this, aListName, aOldFrame); + mAbsoluteContainer.RemoveFrame(this, aListName, aOldFrame); + rv = NS_OK; } else { rv = nsInlineFrame::RemoveFrame(aListName, aOldFrame); } diff --git a/layout/generic/nsViewportFrame.cpp b/layout/generic/nsViewportFrame.cpp index 830fda71612a..7a45a4df8dc5 100644 --- a/layout/generic/nsViewportFrame.cpp +++ b/layout/generic/nsViewportFrame.cpp @@ -156,7 +156,8 @@ ViewportFrame::RemoveFrame(nsIAtom* aListName, nsresult rv = NS_OK; if (nsGkAtoms::fixedList == aListName) { - rv = mFixedContainer.RemoveFrame(this, aListName, aOldFrame); + mFixedContainer.RemoveFrame(this, aListName, aOldFrame); + rv = NS_OK; } else { NS_ASSERTION(!aListName, "unexpected child list"); diff --git a/layout/svg/base/src/nsSVGContainerFrame.cpp b/layout/svg/base/src/nsSVGContainerFrame.cpp index 7761279a8805..39da949bb9cb 100644 --- a/layout/svg/base/src/nsSVGContainerFrame.cpp +++ b/layout/svg/base/src/nsSVGContainerFrame.cpp @@ -84,7 +84,8 @@ nsSVGContainerFrame::RemoveFrame(nsIAtom* aListName, { NS_ASSERTION(!aListName, "unexpected child list"); - return mFrames.DestroyFrame(aOldFrame) ? NS_OK : NS_ERROR_FAILURE; + mFrames.DestroyFrame(aOldFrame); + return NS_OK; } NS_IMETHODIMP diff --git a/layout/tables/nsTableColGroupFrame.cpp b/layout/tables/nsTableColGroupFrame.cpp index 2560fe5c3ae5..abe9cf4dc27e 100644 --- a/layout/tables/nsTableColGroupFrame.cpp +++ b/layout/tables/nsTableColGroupFrame.cpp @@ -299,17 +299,16 @@ nsTableColGroupFrame::RemoveChild(nsTableColFrame& aChild, colIndex = aChild.GetColIndex(); nextChild = aChild.GetNextSibling(); } - if (mFrames.DestroyFrame((nsIFrame*)&aChild)) { - mColCount--; - if (aResetSubsequentColIndices) { - if (nextChild) { // reset inside this and all following colgroups - ResetColIndices(this, colIndex, nextChild); - } - else { - nsIFrame* nextGroup = GetNextSibling(); - if (nextGroup) // reset next and all following colgroups - ResetColIndices(nextGroup, colIndex); - } + mFrames.DestroyFrame(&aChild); + mColCount--; + if (aResetSubsequentColIndices) { + if (nextChild) { // reset inside this and all following colgroups + ResetColIndices(this, colIndex, nextChild); + } + else { + nsIFrame* nextGroup = GetNextSibling(); + if (nextGroup) // reset next and all following colgroups + ResetColIndices(nextGroup, colIndex); } }