Closed Bug 504524 Opened 11 years ago Closed 10 years ago
Don't dirty all textframe continuations when text is inserted
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.
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.
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 :-).
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.
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+
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
But I did leave the CharacterDataChanged patch in: http://hg.mozilla.org/mozilla-central/rev/acba6709fc4a
10 years ago
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
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://email@example.com/ (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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
10 years ago
roc: does this block?
No. It's a performance improvement and a somewhat risky one.
Flags: blocking1.9.2? → blocking1.9.2-
You need to log in before you can comment on or make changes to this bug.