Closed
Bug 410857
Opened 16 years ago
Closed 7 years ago
DrainOverflowLines before ResolveBidi
Categories
(Core :: Layout: Block and Inline, enhancement)
Core
Layout: Block and Inline
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
Details
Attachments
(4 files)
59 bytes,
text/x-review-board-request
|
jfkthame
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
Followup from bug 406380 comment 11. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.917&root=/cvsroot&mark=912,915,924#847 We should move the "DrainOverflowLines" block before "ResolveBidi" block if we can - this way we would have a simpler frame tree to deal with during bidi resolution and possibly create fewer frames in some cases.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
> ... set it to the right block continuation this time
FYI, the error I'm alluding to is this (debug) code:
- aBlockFrame->List(stdout, 0);
+ aBpd->mCurrentBlock->List(stdout, 0);
which currently prints the first-in-flow frame tree, rather than the current
continuation that we're processing.
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8838998 [details] Bug 410857 part 1 - Traverse overflow lines too so we don't miss some text. https://reviewboard.mozilla.org/r/113748/#review115370 ::: layout/base/nsBidiPresUtils.cpp:700 (Diff revision 1) > nsBlockInFlowLineIterator lineIter(block, block->LinesBegin()); > bpd.mPrevFrame = nullptr; > TraverseFrames(aBlockFrame, &lineIter, block->PrincipalChildList().FirstChild(), &bpd); > - // XXX what about overflow lines? > + nsBlockFrame::FrameLines* overflowLines = block->GetOverflowLines(); > + if (overflowLines) { > + nsBlockInFlowLineIterator lineIter(block, overflowLines->mLines.begin(), true); This won't compile as it stands; the `friend` declaration that's in the following patch should be brought over to this one. ::: layout/base/nsBidiPresUtils.cpp:700 (Diff revision 1) > + nsBlockInFlowLineIterator lineIter(block, overflowLines->mLines.begin(), true); > + TraverseFrames(aBlockFrame, &lineIter, block->PrincipalChildList().FirstChild(), &bpd); Please wrap the lines to stay inside the 80-char limit. (I know there's an over-long line nearby already, but we shouldn't be adding more.)
Attachment #8838998 -
Flags: review?(jfkthame) → review+
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8838999 [details] Bug 410857 part 2 - DrainOverflowLines before ResolveBidi for slightly improved performance. https://reviewboard.mozilla.org/r/113750/#review115378 ::: layout/generic/nsBlockFrame.h:1022 (Diff revision 1) > + friend class nsBidiPresUtils; > // XXX nsBlockFrame uses this internally in one place. Try to remove it. > + // XXX uhm, and nsBidiPresUtils::Resolve too. Move to previous patch.
Attachment #8838999 -
Flags: review?(jfkthame) → review+
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8839000 [details] Bug 410857 part 3 - Cache the PresContext instead of fetching it for every paragraph. https://reviewboard.mozilla.org/r/113752/#review115380
Attachment #8839000 -
Flags: review?(jfkthame) → review+
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8839001 [details] Bug 410857 part 4 - Stop passing around aBlockFrame just for DEBUG purposes. Introduce BidiParagraphData::mCurrentBlock for that purpose and set it to the right block continuation this time. https://reviewboard.mozilla.org/r/113754/#review115382
Attachment #8839001 -
Flags: review?(jfkthame) → review+
Comment 10•7 years ago
|
||
Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5675fc7b6112 part 1 - Traverse overflow lines too so we don't miss some text. r=jfkthame https://hg.mozilla.org/integration/mozilla-inbound/rev/76f8f7b04aa8 part 2 - DrainOverflowLines before ResolveBidi for slightly improved performance. r=jfkthame https://hg.mozilla.org/integration/mozilla-inbound/rev/f7605fcc9b38 part 3 - Cache the PresContext instead of fetching it for every paragraph. r=jfkthame https://hg.mozilla.org/integration/mozilla-inbound/rev/4993c79562fe part 4 - Stop passing around aBlockFrame just for DEBUG purposes. Introduce BidiParagraphData::mCurrentBlock for that purpose and set it to the right block continuation this time. r=jfkthame
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5675fc7b6112 https://hg.mozilla.org/mozilla-central/rev/76f8f7b04aa8 https://hg.mozilla.org/mozilla-central/rev/f7605fcc9b38 https://hg.mozilla.org/mozilla-central/rev/4993c79562fe
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 12•7 years ago
|
||
Comment on attachment 8838998 [details] Bug 410857 part 1 - Traverse overflow lines too so we don't miss some text. Approval Request Comment [Feature/Bug causing the regression]: [User impact if declined]: correctness and performance issues on sites with bidi content [Is this code covered by automated tests?]: I'm pretty sure we have many tests covering these code paths already [Has the fix been verified in Nightly?]: [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: all parts in this bug [Is the change risky?]:low risk [Why is the change risky/not risky?]:it adds no new code really, just runs existing code in slightly different way [String changes made/needed]:none
Attachment #8838998 -
Flags: approval-mozilla-esr52?
Attachment #8838998 -
Flags: approval-mozilla-beta?
Attachment #8838998 -
Flags: approval-mozilla-aurora?
Comment 13•7 years ago
|
||
Comment on attachment 8838998 [details] Bug 410857 part 1 - Traverse overflow lines too so we don't miss some text. We're a week from RC, not convinced this needs to jump the trains quite that fast. Am I missing something?
Attachment #8838998 -
Flags: approval-mozilla-esr52?
Attachment #8838998 -
Flags: approval-mozilla-beta?
Attachment #8838998 -
Flags: approval-mozilla-beta-
Comment 14•7 years ago
|
||
Comment on attachment 8838998 [details] Bug 410857 part 1 - Traverse overflow lines too so we don't miss some text. jfkthame explained the reasoning out of band. aurora53+, beta52+, these should be in b9.
Attachment #8838998 -
Flags: approval-mozilla-beta-
Attachment #8838998 -
Flags: approval-mozilla-beta+
Attachment #8838998 -
Flags: approval-mozilla-aurora?
Attachment #8838998 -
Flags: approval-mozilla-aurora+
Comment 15•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/485a44ead9f4 https://hg.mozilla.org/releases/mozilla-aurora/rev/d83db77b92f8 https://hg.mozilla.org/releases/mozilla-aurora/rev/0fa7c14ad50d https://hg.mozilla.org/releases/mozilla-aurora/rev/2022fb5a3a0e
status-firefox53:
--- → fixed
Comment 16•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/4619a89b7f73 https://hg.mozilla.org/releases/mozilla-beta/rev/720cbc37da35 https://hg.mozilla.org/releases/mozilla-beta/rev/b3fc8d044dec https://hg.mozilla.org/releases/mozilla-beta/rev/0316bb85a29c
status-firefox52:
--- → fixed
Comment 17•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/4619a89b7f73 https://hg.mozilla.org/releases/mozilla-esr52/rev/720cbc37da35 https://hg.mozilla.org/releases/mozilla-esr52/rev/b3fc8d044dec https://hg.mozilla.org/releases/mozilla-esr52/rev/0316bb85a29c
status-firefox-esr52:
--- → fixed
Comment 18•7 years ago
|
||
Setting qe-verify- based on Mats' assessment on manual testing needs (see Comment 12) and the fact that this fix has automated coverage.
Flags: qe-verify-
Assignee | ||
Comment 19•7 years ago
|
||
Ouch, looks like this caused a crash regression: bug 1343606. :-(
You need to log in
before you can comment on or make changes to this bug.
Description
•