Don't dirty all textframe continuations when text is inserted

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
wanted-next +
blocking1.9.2 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Currently when text is inserted before the end of a text node, we dirty the textframe containing the insertion and all the following continuations. That's overkill, and if you have a large amount of text in a single textnode being edited, it will be bad for performance.
Currently nsTextFrame::CharacterDataChanged loops over text frame continuations marking them with NS_FRAME_IS_DIRTY. That's actually bad, right? Because we're not calling FrameNeedsReflow and the parent frames aren't being marked with NS_FRAME_HAS_DIRTY_CHILDREN. I guess it's "working" because we never skip reflowing the non-dirty children of inline elements so NS_FRAME_HAS_DIRTY_CHILDREN doesn't matter for them. We're not marking the lines dirty either, but things work out because right now whenever we reflow an inline frame that's not complete we mark the next line dirty. Of course I'm going to have to change that here.

Actually marking all affected textframes with FrameNeedsReflow would be pretty expensive though, for a large change in the text ... a large number of calls to MarkIntrinsicWidthsDirty on all ancestors, for example. Maybe an acceptable optimization would be to not call FrameNeedsReflow if the continuation has the same parent as the previous continuation we already called FrameNeedsReflow on.
Created attachment 388896 [details]
testcase

Load this testcase with the HTML5. You'll get 100,000 lines in a single text node. Try inserting and deleting characters at the top, middle, and bottom.
Created attachment 388898 [details] [diff] [review]
fix

With this patch I can type or delete characters anywhere in those lines and only a few lines are reflowed. Need to run reftests and do more testing.
It might be nice to add a comment explaining why after a mutation the content offsets are such for all the frames we haven't marked dirty yet that if another mutation comes along we'll mark the right frames dirty....  Plus adding testcases to that effect, of course.
Is this going to be OK with bidi? For example, if you have lots of lines of neutral characters and you insert a right-to-left character at the beginning, all the following characters will need to change direction.
If bidi is enabled, then every mutation causes bidi re-resolution to run. Won't that ensure everything is OK?
Patch seems to pass reftests as-is.
(In reply to comment #4)
> It might be nice to add a comment explaining why after a mutation the content
> offsets are such for all the frames we haven't marked dirty yet that if another
> mutation comes along we'll mark the right frames dirty....  Plus adding
> testcases to that effect, of course.

I'll add tests for that, but I'm not sure why the multiple mutation case needs to be specially commented upon. The offsets for the non-dirty frames have to be updated correctly, otherwise basic things like painting would break.
I guess it wasn't obvious to me on skimming the patch why the offset manipulations performed are the right ones; I'll have to read it more carefully.
Ah I see what you mean.

After writing some more tests, I found at least one bug in the offset manipulations. So it shouldn't have been obvious :-).
Created attachment 389084 [details] [diff] [review]
fix v2

Ah no, the bug was somewhere else ... we weren't reflowing lines that contain text frames dirtied by CharacterDataChanged, because the lines weren't dirty.

I added some comments to try to help understand what's going on with the character offset changes. And tests.
Attachment #388898 - Attachment is obsolete: true
Attachment #389084 - Flags: review?(dbaron)
Comment on attachment 389084 [details] [diff] [review]
fix v2

r=bzbarsky with the other SplitLine callsite's MarkDirty() also nixed.
Attachment #389084 - Flags: review?(dbaron) → review+
Created attachment 391537 [details] [diff] [review]
prepatch

I forgot to request review on this pre-patch that changes the signature of nsIFrame::CharacterDataChanged
Attachment #391537 - Flags: review?(bzbarsky)
Attachment #391537 - Flags: review?(bzbarsky) → review+
I checked this in but backed out due to failures in test_bug332655-2.html on Mac and Linux:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249440415.1249446860.29769.gz#err1
Blocks: 508334
Flags: wanted-next+
These are the test failures:
43221 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug332655-2.html | deleting and inserting RTL char at beginning of line shouldn't change 
43222 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug332655-2.html | the order entering Bidi text shouldn't change rendering
Blocks: 501566
So I applied the patch locally to see if I could help with this, and it turns out that the regression is fixed by my patch for bug 514156. Pushing both patches to tryserver now.
Depends on: 514156
Mmm, that would be great! Thanks!
At the third attempt, all tryserver tests passed. Builds are at https://build.mozilla.org/tryserver-builds/smontagu@mozilla.com-try-1f8a5adc822d/ (The first time I forgot to hg add the reftests and the second time failed because there were errors in the filenames in reftests/text/reftest.list). The push at http://hg.mozilla.org/try/rev/819363eae9b1 also includes a couple of small changes to make the patch apply to current trunk.
Blocks: 515696
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 523460
Depends on: 523468
No longer blocks: 508334
Depends on: 524284
Depends on: 525582

Updated

9 years ago
Depends on: 525986
Flags: blocking1.9.2?

Comment 21

9 years ago
This might benefit from bug 240933, so adding a dependency for now.
Depends on: 240933
Blocks: 240933
No longer depends on: 240933
roc: does this block?
No. It's a performance improvement and a somewhat risky one.
Flags: blocking1.9.2? → blocking1.9.2-

Updated

9 years ago
Depends on: 531603
Depends on: 534366
Blocks: 547860
You need to log in before you can comment on or make changes to this bug.