Closed Bug 646359 Opened 9 years ago Closed 6 years ago

Optimize bidi resolution on lines with single-direction text

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: smontagu, Assigned: smontagu)

References

(Blocks 4 open bugs)

Details

(Keywords: perf, Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

I originally included a patch to do this in attachment 495873 [details] [diff] [review] as part of bug 263359 but it caused bug 654419.

It's a very significant performance win in large files where most lines are left-to-right but there are a few lines containing right-to-left text -- tinderbox logs are the classic example for developers.
Keywords: perf
Attached patch Patch (obsolete) — Splinter Review
Bug 645119 was caused by only dirtying the line when the directionality of the frame had changed.
Assignee: nobody → smontagu
Attachment #523273 - Flags: review?(roc)
Comment on attachment 523273 [details] [diff] [review]
Patch

Sorry about the delay.

Is there a way to not always mark the line dirty? What is the case in bug 645119 where the line needs to be marked dirty?
Attachment #523273 - Flags: review?(roc) → review+
What is the cost of marking the line dirty? I think that to prevent bug 645119 all we need to do is repaint the line, so if there is a cheaper way to do that it would be sufficient.
Blocks: 650189
No longer blocks: 650189
Marking the line dirty means it will have to be reflowed. Possibly other lines next to it will be reflowed as well.

You can invalidate the line's "combined area" if you want.
Blocks: 722641
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> 
> r+

Hey, a reviewed patch from 2011! What does it need to land?
I think Simon just needs to call SchedulePaint now to ensure that we re-run DLBI and paint if necessary.
All my attempts to call SchedulePaint (or InvalidateFrame or InvalidateFrameSubtree) end up leaving text turds on the screen in the scenario from bug 645119. I'm guessing that this is because when entering or deleting a newline in a text area and making existing lines scroll up or down, the area that gets invalidated is where the line *used* to be, and it doesn't get painted in its new position.
STR: focus the caret at the top of the textarea and press Enter
(In reply to Simon Montagu :smontagu from comment #7)
> All my attempts to call SchedulePaint (or InvalidateFrame or
> InvalidateFrameSubtree) end up leaving text turds on the screen in the
> scenario from bug 645119. I'm guessing that this is because when entering or
> deleting a newline in a text area and making existing lines scroll up or
> down, the area that gets invalidated is where the line *used* to be, and it
> doesn't get painted in its new position.

No, that sort of thing doesn't happen anymore with DLBI.

DLBI doesn't handle text frames being displayed with different contents, so SchedulePaint isn't enough given we're changing the way a textframe renders --- but InvalidateFrame or InvalidateFrameSubtree on the text frame should still work. I don't know why it doesn't...
Blocks: 762710
There's something I don't understand here, and maybe explaining that would indicate a way forward: if I hack the code so that we don't do bidi resolution at all, there is no regression in the testcase in comment 8.

What is bidi doing that prevents the lines repainting properly?
As so often, 90% of the solution was asking the right question: since bug 263359 bidi can create continuation frames after hard line breaks while walking the frames preparing for bidi resolution. When this happens we need to reflow the line just as we do after creating non-fluid continuations during bidi resolution itself.

Try run at https://tbpl.mozilla.org/?tree=Try&rev=6cfaabfdde8d has some remaining issues which I'm fixing.
Depends on: 936935
Attached patch Updated patchSplinter Review
With the patch for bug 936935, there don't seem to be any regressions any more.
Attachment #523273 - Attachment is obsolete: true
Attachment #832100 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/bdf16bdfce5a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 964281
Depends on: 965602
No longer depends on: 965602
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.