Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Description • 16 years ago

nsContainerFrame::DeleteNextInFlowChild calls nsSplittableFrame::BreakFromPrevFlow to disconnect aNextInFlow from its prev-in-flow before calling StealFrame to remove it from its parent's child list and before calling aNextInFlow->Destroy. When aNextInFlow is an nsContinuingTextFrame, this forces nsContinuingTextFrame::Destroy to call ClearTextRun since mPrevContinuation is null. This blows away the text run, probably unnecessarily since all that's happened is that the prev-in-flow text frame has taken up the text that was being mapped by the nsContinuingTextFrame. However, after we've been disconnected from the flow nsContinuingTextFrame::Destroy has no way of knowing whether it's safe to avoid calling ClearTextRun.

We shouldn't have to call BreakFromPrevFlow there because Destroy is going to unlink the frame from the flow (in nsSplittableFrame::Destroy for most frame types). I don't think StealFrame needs to deal with flow-chains (only sibling chains), so I don't know why we're unlinking there. CVS archaeology shows that it's been done there since near the beginning of time.

This stuff is fragile so the patch is risky, but it's worth taking at some point since it reduces textrun reconstruction considerably.

	David Baron :dbaron:
	Comment 1 • 16 years ago

Comment on attachment 349156 [details] [diff] [review]
fix

I think Uri should also look at this.

I'm worried that this will do the wrong thing in case some of the in-flows have fixed continuations (i.e., bidi continuations), given the code in BreakFromPrevInFlow to handle that case.

I'd also note that this leaves the only caller of nsSplittableFrame::BreakPrevInFlow being in obscure :first-letter code.  Does that code really still need to call it if this doesn't?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 2 • 16 years ago

BreakFromPrevInFlow does "if aFrame has a fixed next-continuation, then remove aFrame from the flow chain in both directions, otherwise just break the relationship with its prev-continuation." RemoveFromFlow removes aFrame from the flow chain unconditionally. As far as I can tell, BreakFromPrevInFlow followed by RemoveFromFlow is equivalent to RemoveFromFlow.

I think we can get rid of the call in nsCSSFrameConstructor as well, although I'd do that in a separate patch.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 3 • 16 years ago

Needed for blocker bug 430332.

	David Baron :dbaron:
	Comment 4 • 16 years ago

Comment on attachment 349156 [details] [diff] [review]
fix

(In reply to comment #2)
> As far as I can tell, BreakFromPrevInFlow
> followed by RemoveFromFlow is equivalent to RemoveFromFlow.

OK, so the case where that's not true is actually the case where all the continuations are fluid.  In particular, if you have three fluid continuations:
  A -> B -> C
then the sequence of two calls yields:
  A  [ no connection ]  C
whereas the revised code yields:
  A -> C
since BreakFromPrevInFlow no longer breaks the A->B connection, so RemoveFromFlow connects A and C.

However, given the loop just before (beginning with nextNextInFlow), that isn't really a problem, so this does look fine.  r+sr=dbaron

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 5 • 16 years ago

Pushed 11a86c545296

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 6 • 16 years ago

Comment on attachment 349156 [details] [diff] [review]
fix

After this has baked for a week or two with no regressions, I want to take this for 1.9.1. In combination with other patches, it can reduce textrun reconstruction, speeding up text reflow in some important cases.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 7 • 16 years ago

Comment on attachment 349156 [details] [diff] [review]
fix

a191=beltzner

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 8 • 16 years ago

We shouldn't land this on 1.9.1 until we have a fix for bug 470272.

	Martijn Wargers (dead)
	Comment 9 • 16 years ago

There is supposedly a fix in bug 470978.