410857 - DrainOverflowLines before ResolveBidi

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

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 1

[Mats Palmgren (inactive)]

Attachment: Bug 410857 part 1 - Traverse overflow lines too so we don't miss some text.

Review commit: https://reviewboard.mozilla.org/r/113748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113748/

Comment 2

[Mats Palmgren (inactive)]

Attachment: Bug 410857 part 2 - DrainOverflowLines before ResolveBidi for slightly improved performance.

Review commit: https://reviewboard.mozilla.org/r/113750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113750/

Comment 3

[Mats Palmgren (inactive)]

Attachment: Bug 410857 part 3 - Cache the PresContext instead of fetching it for every paragraph.

Review commit: https://reviewboard.mozilla.org/r/113752/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113752/

Comment 4

[Mats Palmgren (inactive)]

Attachment: 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.

Review commit: https://reviewboard.mozilla.org/r/113754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113754/

Comment 5

[Mats Palmgren (inactive)]

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

[Jonathan Kew [:jfkthame]]

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

Comment 7

[Jonathan Kew [:jfkthame]]

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.

Comment 8

[Jonathan Kew [:jfkthame]]

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

Comment 9

[Jonathan Kew [:jfkthame]]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

[Phil Ringnalda (:philor)]

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

Comment 12

[Mats Palmgren (inactive)]

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

Comment 13

[Julien Cristau [:jcristau]]

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?

Comment 14

[Julien Cristau [:jcristau]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Mats' assessment on manual testing needs (see Comment 12) and the fact that this fix has automated coverage.

Comment 19

[Mats Palmgren (inactive)]

Ouch, looks like this caused a crash regression: bug 1343606. :-(