DrainOverflowLines before ResolveBidi

RESOLVED FIXED in Firefox 52

Status

()

Core
Layout: Block and Inline
--
enhancement
RESOLVED FIXED
11 years ago
a year ago

People

(Reporter: mats, Assigned: mats)

Tracking

Trunk
mozilla54
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

11 years ago
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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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 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 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+
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-
Ouch, looks like this caused a crash regression: bug 1343606. :-(
Depends on: 1343606
You need to log in before you can comment on or make changes to this bug.