Closed
Bug 504524
Opened 15 years ago
Closed 15 years ago
Don't dirty all textframe continuations when text is inserted
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: roc)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
185 bytes,
text/html
|
Details | |
17.97 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
14.08 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
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.
Assignee | ||
Comment 2•15 years ago
|
||
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.
Assignee | ||
Comment 3•15 years ago
|
||
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.
Comment 4•15 years ago
|
||
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.
Comment 5•15 years ago
|
||
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.
Assignee | ||
Comment 6•15 years ago
|
||
If bidi is enabled, then every mutation causes bidi re-resolution to run. Won't that ensure everything is OK?
Assignee | ||
Comment 7•15 years ago
|
||
Patch seems to pass reftests as-is.
Assignee | ||
Comment 8•15 years ago
|
||
(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.
Comment 9•15 years ago
|
||
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.
Assignee | ||
Comment 10•15 years ago
|
||
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 :-).
Assignee | ||
Comment 11•15 years ago
|
||
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 12•15 years ago
|
||
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+
Assignee | ||
Comment 13•15 years ago
|
||
I forgot to request review on this pre-patch that changes the signature of nsIFrame::CharacterDataChanged
Attachment #391537 -
Flags: review?(bzbarsky)
Updated•15 years ago
|
Attachment #391537 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 14•15 years ago
|
||
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
Assignee | ||
Comment 15•15 years ago
|
||
But I did leave the CharacterDataChanged patch in: http://hg.mozilla.org/mozilla-central/rev/acba6709fc4a
Assignee | ||
Updated•15 years ago
|
Flags: wanted-next+
Assignee | ||
Comment 16•15 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
Comment 17•15 years ago
|
||
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
Assignee | ||
Comment 18•15 years ago
|
||
Mmm, that would be great! Thanks!
Comment 19•15 years ago
|
||
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.
Comment 20•15 years ago
|
||
Repushed http://hg.mozilla.org/mozilla-central/rev/d3298de30066
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Updated•15 years ago
|
Flags: blocking1.9.2?
Comment 21•15 years ago
|
||
This might benefit from bug 240933, so adding a dependency for now.
Depends on: 240933
Assignee | ||
Updated•15 years ago
|
Comment 22•15 years ago
|
||
roc: does this block?
Assignee | ||
Comment 23•15 years ago
|
||
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.
Description
•