From 0fe5aef6c28ce94c3f2bfa0809a1ce6237d33991 Mon Sep 17 00:00:00 2001 From: "roc+@cs.cmu.edu" Date: Mon, 3 Dec 2007 00:22:07 -0800 Subject: [PATCH] Bug 403426. We should clear whitespace status when we reset the linebreaker. If a final break opportunity exists we should save it and forward it to the line layout. Relanding with a fix so hopefully we won't crash Tp this time. r=smontagu --- content/base/public/nsLineBreaker.h | 5 +- content/base/src/nsLineBreaker.cpp | 18 ++++++- layout/generic/nsTextFrameThebes.cpp | 68 +++++++++++++++++++------- layout/generic/nsTextFrameUtils.h | 7 ++- layout/reftests/bugs/403426-1-ref.html | 8 +++ layout/reftests/bugs/403426-1.html | 9 ++++ layout/reftests/bugs/reftest.list | 1 + 7 files changed, 93 insertions(+), 23 deletions(-) create mode 100644 layout/reftests/bugs/403426-1-ref.html create mode 100644 layout/reftests/bugs/403426-1.html diff --git a/content/base/public/nsLineBreaker.h b/content/base/public/nsLineBreaker.h index fc507479e69..8ce15683e8e 100644 --- a/content/base/public/nsLineBreaker.h +++ b/content/base/public/nsLineBreaker.h @@ -184,8 +184,11 @@ public: * After this call, this linebreaker can be reused. * This must be called at least once between any call to AppendText() and * destroying the object. + * @param aTrailingBreak this is set to true when there is a break opportunity + * at the end of the text. This will normally only be declared true when there + * is breakable whitespace at the end. */ - nsresult Reset() { return FlushCurrentWord(); } + nsresult Reset(PRBool* aTrailingBreak); private: // This is a list of text sources that make up the "current word" (i.e., diff --git a/content/base/src/nsLineBreaker.cpp b/content/base/src/nsLineBreaker.cpp index 1384e0167c2..c0a3a5aba0c 100644 --- a/content/base/src/nsLineBreaker.cpp +++ b/content/base/src/nsLineBreaker.cpp @@ -391,7 +391,8 @@ nsLineBreaker::AppendText(nsIAtom* aLangGroup, const PRUint8* aText, PRUint32 aL } nsresult -nsLineBreaker::AppendInvisibleWhitespace(PRUint32 aFlags) { +nsLineBreaker::AppendInvisibleWhitespace(PRUint32 aFlags) +{ nsresult rv = FlushCurrentWord(); if (NS_FAILED(rv)) return rv; @@ -401,5 +402,18 @@ nsLineBreaker::AppendInvisibleWhitespace(PRUint32 aFlags) { mBreakHere = PR_TRUE; } mAfterBreakableSpace = isBreakableSpace; - return NS_OK; + return NS_OK; +} + +nsresult +nsLineBreaker::Reset(PRBool* aTrailingBreak) +{ + nsresult rv = FlushCurrentWord(); + if (NS_FAILED(rv)) + return rv; + + *aTrailingBreak = mBreakHere || mAfterBreakableSpace; + mBreakHere = PR_FALSE; + mAfterBreakableSpace = PR_FALSE; + return NS_OK; } diff --git a/layout/generic/nsTextFrameThebes.cpp b/layout/generic/nsTextFrameThebes.cpp index d7466b45280..492fd49c12a 100644 --- a/layout/generic/nsTextFrameThebes.cpp +++ b/layout/generic/nsTextFrameThebes.cpp @@ -603,10 +603,16 @@ public: mDoubleByteText = PR_FALSE; } void ResetLineBreaker() { - mLineBreaker.Reset(); + PRBool trailingBreak; + mLineBreaker.Reset(&trailingBreak); } void AccumulateRunInfo(nsTextFrame* aFrame); - void BuildTextRunForFrames(void* aTextBuffer); + /** + * @return null to indicate either textrun construction failed or + * we constructed just a partial textrun to set up linebreaker and other + * state for following textruns. + */ + gfxTextRun* BuildTextRunForFrames(void* aTextBuffer); void AssignTextRun(gfxTextRun* aTextRun); nsTextFrame* GetNextBreakBeforeFrame(PRUint32* aIndex); void SetupBreakSinksForTextRun(gfxTextRun* aTextRun, PRBool aIsExistingTextRun, @@ -691,9 +697,8 @@ private: gfxContext* mContext; nsIFrame* mLineContainer; nsTextFrame* mLastFrame; - // The common ancestor of the current frame and the previous text frame - // on the line, if there's no non-text frame boundaries in between. Otherwise - // null. + // The common ancestor of the current frame and the previous leaf frame + // on the line, or null if there was no previous leaf frame. nsIFrame* mCommonAncestorWithLastFrame; // mMaxTextLength is an upper bound on the size of the text in all mapped frames PRUint32 mMaxTextLength; @@ -1028,26 +1033,35 @@ void BuildTextRunsScanner::FlushFrames(PRBool aFlushLineBreaks) if (mMappedFlows.Length() == 0) return; + gfxTextRun* textRun; if (!mSkipIncompleteTextRuns && mCurrentFramesAllSameTextRun && ((mCurrentFramesAllSameTextRun->GetFlags() & nsTextFrameUtils::TEXT_INCOMING_WHITESPACE) != 0) == mCurrentRunTrimLeadingWhitespace && IsTextRunValidForMappedFlows(mCurrentFramesAllSameTextRun)) { // Optimization: We do not need to (re)build the textrun. + textRun = mCurrentFramesAllSameTextRun; // Feed this run's text into the linebreaker to provide context. This also // updates mTrimNextRunLeadingWhitespace appropriately. - SetupBreakSinksForTextRun(mCurrentFramesAllSameTextRun, PR_TRUE, PR_FALSE); + SetupBreakSinksForTextRun(textRun, PR_TRUE, PR_FALSE); mTrimNextRunLeadingWhitespace = - (mCurrentFramesAllSameTextRun->GetFlags() & nsTextFrameUtils::TEXT_TRAILING_WHITESPACE) != 0; + (textRun->GetFlags() & nsTextFrameUtils::TEXT_TRAILING_WHITESPACE) != 0; } else { nsAutoTArray buffer; if (!buffer.AppendElements(mMaxTextLength*(mDoubleByteText ? 2 : 1))) return; - BuildTextRunForFrames(buffer.Elements()); + textRun = BuildTextRunForFrames(buffer.Elements()); } if (aFlushLineBreaks) { - mLineBreaker.Reset(); + PRBool trailingLineBreak; + nsresult rv = mLineBreaker.Reset(&trailingLineBreak); + // textRun may be null for various reasons, including because we constructed + // a partial textrun just to get the linebreaker and other state set up + // to build the next textrun. + if (NS_SUCCEEDED(rv) && trailingLineBreak && textRun) { + textRun->SetFlagBits(nsTextFrameUtils::TEXT_HAS_TRAILING_BREAK); + } PRUint32 i; for (i = 0; i < mBreakSinks.Length(); ++i) { if (!mBreakSinks[i]->mExistingTextRun || mBreakSinks[i]->mChangedBreaks) { @@ -1067,7 +1081,7 @@ void BuildTextRunsScanner::AccumulateRunInfo(nsTextFrame* aFrame) mMaxTextLength += aFrame->GetContentLength(); mDoubleByteText |= aFrame->GetContent()->GetText()->Is2b(); mLastFrame = aFrame; - mCommonAncestorWithLastFrame = aFrame; + mCommonAncestorWithLastFrame = aFrame->GetParent(); MappedFlow* mappedFlow = &mMappedFlows[mMappedFlows.Length() - 1]; NS_ASSERTION(mappedFlow->mStartFrame == aFrame || @@ -1198,12 +1212,12 @@ void BuildTextRunsScanner::ScanFrame(nsIFrame* aFrame) PRBool descendInto = PR_TRUE; if (!continueTextRun) { FlushFrames(PR_TRUE); - mCommonAncestorWithLastFrame = nsnull; + mCommonAncestorWithLastFrame = aFrame; + mTrimNextRunLeadingWhitespace = PR_FALSE; // XXX do we need this? are there frames we need to descend into that aren't // float-containing-blocks? descendInto = !aFrame->IsFloatContainingBlock(); mStartOfLine = PR_FALSE; - mTrimNextRunLeadingWhitespace = PR_FALSE; } if (descendInto) { @@ -1215,7 +1229,7 @@ void BuildTextRunsScanner::ScanFrame(nsIFrame* aFrame) if (!continueTextRun) { FlushFrames(PR_TRUE); - mCommonAncestorWithLastFrame = nsnull; + mCommonAncestorWithLastFrame = aFrame; mTrimNextRunLeadingWhitespace = PR_FALSE; } @@ -1322,7 +1336,7 @@ GetFontMetrics(gfxFontGroup* aFontGroup) return font->GetMetrics(); } -void +gfxTextRun* BuildTextRunsScanner::BuildTextRunForFrames(void* aTextBuffer) { gfxSkipCharsBuilder builder; @@ -1421,7 +1435,7 @@ BuildTextRunsScanner::BuildTextRunForFrames(void* aTextBuffer) nsAutoTArray tempBuf; if (!tempBuf.AppendElements(contentLength)) { DestroyUserData(userData); - return; + return nsnull; } PRUint8* bufStart = tempBuf.Elements(); PRUint8* end = nsTextFrameUtils::TransformText( @@ -1456,7 +1470,7 @@ BuildTextRunsScanner::BuildTextRunForFrames(void* aTextBuffer) // Check for out-of-memory in gfxSkipCharsBuilder if (!builder.IsOK()) { DestroyUserData(userData); - return; + return nsnull; } void* finalUserData; @@ -1491,7 +1505,7 @@ BuildTextRunsScanner::BuildTextRunForFrames(void* aTextBuffer) gfxFontGroup* fontGroup = GetFontGroupForFrame(firstFrame); if (!fontGroup) { DestroyUserData(userData); - return; + return nsnull; } if (textFlags & nsTextFrameUtils::TEXT_HAS_TAB) { @@ -1598,7 +1612,7 @@ BuildTextRunsScanner::BuildTextRunForFrames(void* aTextBuffer) } if (!textRun) { DestroyUserData(userData); - return; + return nsnull; } // We have to set these up after we've created the textrun, because @@ -1615,12 +1629,13 @@ BuildTextRunsScanner::BuildTextRunForFrames(void* aTextBuffer) gTextRuns->RemoveFromCache(textRun); delete textRun; DestroyUserData(userData); - return; + return nsnull; } // Actually wipe out the textruns associated with the mapped frames and associate // those frames with this text run. AssignTextRun(textRun); + return textRun; } static PRBool @@ -5508,6 +5523,19 @@ nsTextFrame::Reflow(nsPresContext* aPresContext, lineLayout.NotifyOptionalBreakPosition(mContent, offset + length, textMetrics.mAdvanceWidth + provider.GetHyphenWidth() <= availWidth); } + PRBool breakAfter = PR_FALSE; + if ((charsFit == length && transformedOffset + transformedLength == mTextRun->GetLength() && + (mTextRun->GetFlags() & nsTextFrameUtils::TEXT_HAS_TRAILING_BREAK))) { + // Note that because we didn't break, we can be sure that (thanks to the + // code up above) textMetrics.mAdvanceWidth includes the width of any + // trailing whitespace. So we need to subtract trimmableWidth here + // because if we did break at this point, that much width would be trimmed. + if (textMetrics.mAdvanceWidth - trimmableWidth > availWidth) { + breakAfter = PR_TRUE; + } else { + lineLayout.NotifyOptionalBreakPosition(mContent, offset + length, PR_TRUE); + } + } if (completedFirstLetter) { lineLayout.SetFirstLetterStyleOK(PR_FALSE); } @@ -5523,6 +5551,8 @@ nsTextFrame::Reflow(nsPresContext* aPresContext, // Ends in \n aStatus = NS_INLINE_LINE_BREAK_AFTER(aStatus); lineLayout.SetLineEndsInBR(PR_TRUE); + } else if (breakAfter) { + aStatus = NS_INLINE_LINE_BREAK_AFTER(aStatus); } // Compute space and letter counts for justification, if required diff --git a/layout/generic/nsTextFrameUtils.h b/layout/generic/nsTextFrameUtils.h index 26d1385af3a..9e1bd1bde0c 100644 --- a/layout/generic/nsTextFrameUtils.h +++ b/layout/generic/nsTextFrameUtils.h @@ -73,7 +73,12 @@ public: TEXT_TRAILING_WHITESPACE = 0x400000, TEXT_COMPRESSED_LEADING_WHITESPACE = 0x800000, TEXT_NO_BREAKS = 0x1000000, - TEXT_IS_TRANSFORMED = 0x2000000 + TEXT_IS_TRANSFORMED = 0x2000000, + // This gets set if there's a break opportunity at the end of the textrun. + // We normally don't use this break opportunity because the following text + // will have a break opportunity at the start, but it's useful for line + // layout to know about it in case the following content is not text + TEXT_HAS_TRAILING_BREAK = 0x4000000 }; /** diff --git a/layout/reftests/bugs/403426-1-ref.html b/layout/reftests/bugs/403426-1-ref.html new file mode 100644 index 00000000000..3e28517475d --- /dev/null +++ b/layout/reftests/bugs/403426-1-ref.html @@ -0,0 +1,8 @@ + + + +

hello1 +

Hello2
+

Hello3
+ + diff --git a/layout/reftests/bugs/403426-1.html b/layout/reftests/bugs/403426-1.html new file mode 100644 index 00000000000..8e93d6f3a06 --- /dev/null +++ b/layout/reftests/bugs/403426-1.html @@ -0,0 +1,9 @@ + + + +

hello1 +

Hello2 +

Hello3 + + + diff --git a/layout/reftests/bugs/reftest.list b/layout/reftests/bugs/reftest.list index da60c95dbfc..b804b73e95b 100644 --- a/layout/reftests/bugs/reftest.list +++ b/layout/reftests/bugs/reftest.list @@ -473,6 +473,7 @@ random == 403134-1.html 403134-1-ref.html # bug 405377 == 403249-1b.html 403249-1-ref.html == 403249-2a.html 403249-2-ref.html == 403249-2b.html 403249-2-ref.html +== 403426-1.html 403426-1-ref.html == 403455-1.html 403455-1-ref.html == 403505-1.xml 403505-1-ref.xul == 403519-1.html 403519-1-ref.html