Closed
Bug 465913
Opened 16 years ago
Closed 16 years ago
Don't BreakFromPrevFlow in nsContainerFrame::DeleteNextInFlowChild
Categories
(Core :: Layout, defect, P3)
Core
Layout
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: roc)
References
Details
Attachments
(1 file)
958 bytes,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
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.
Attachment #349156 -
Flags: superreview?(dbaron)
Attachment #349156 -
Flags: review?(dbaron)
Attachment #349156 -
Flags: review?(uriber)
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?
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.
Assignee | ||
Updated•16 years ago
|
Flags: blocking1.9.1? → wanted1.9.1+
Priority: -- → P3
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review]
Attachment #349156 -
Flags: superreview?(dbaron)
Attachment #349156 -
Flags: superreview+
Attachment #349156 -
Flags: review?(uriber)
Attachment #349156 -
Flags: review?(dbaron)
Attachment #349156 -
Flags: review+
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
Whiteboard: [needs review] → [needs landing]
Assignee | ||
Comment 5•16 years ago
|
||
Pushed 11a86c545296
Whiteboard: [needs landing] → [needs 191 approval]
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.
Attachment #349156 -
Flags: approval1.9.1?
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 7•16 years ago
|
||
Comment on attachment 349156 [details] [diff] [review] fix a191=beltzner
Attachment #349156 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Comment 8•16 years ago
|
||
We shouldn't land this on 1.9.1 until we have a fix for bug 470272.
Whiteboard: [needs 191 approval]
Assignee | ||
Updated•16 years ago
|
Flags: wanted1.9.1+ → wanted1.9.1-
Comment 9•16 years ago
|
||
There is supposedly a fix in bug 470978.
Updated•16 years ago
|
Attachment #349156 -
Flags: approval1.9.1+
You need to log in
before you can comment on or make changes to this bug.
Description
•