Closed Bug 410857 Opened 16 years ago Closed 7 years ago

DrainOverflowLines before ResolveBidi

Categories

(Core :: Layout: Block and Inline, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

Details

Attachments

(4 files)

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.
> ... 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 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 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 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 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+
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 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.