Closed Bug 593211 Opened 10 years ago Closed 10 years ago

Optimize caret movement without caching the caret frame and frame offset

Categories

(Core :: DOM: Editor, defect, major)

x86
Windows 7
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: jmjjeffery, Assigned: ehsan)

References

()

Details

(Keywords: perf)

Attachments

(5 files, 8 obsolete files)

Go to the URL above, or even use Bugzilla - as I'm seeing the issue here also:

1. Enter text into the Description box and note that the cursor does not follow the typing, and only plays catch-up when you stop/pause typing. 

Tested using the latest hourly build based on cset: 
http://hg.mozilla.org/mozilla-central/rev/ba1849f86ade

Not sure what bug caused the regression, any of:
bug 240933 , bug 570192 , bug 590554
I don't know how on Earth I managed to miss this.  Anyway, I've backed out bug 240933 for now.
Assignee: nobody → ehsan
Blocks: 240933
Status: NEW → ASSIGNED
Duplicate of this bug: 593223
bisect tells me that this is a regression from attachment 459123 [details] [diff] [review].  I'm going to investigate (and fix) this tomorrow.
This is actually a regression from bug 581585.  When we try to use the cached frame and frame offset for the caret, we don't check to see if the content subtree for mLastContent is the same as it was before the call.  I have a test case which demonstrates this happening without even using a textarea.
Blocks: 581585
In this test case, we put the selection on the second child of the div, and then mutate the first child.  The caret doesn't update its position after this mutation.

This works correctly before bug 581585.  I also tested this on Chrome, which was affected by the same bug.
Attached patch WIP (obsolete) — Splinter Review
This is a WIP, which mostly works, but it crashes when running the editor tests.  The crash happens when nsNodeUtils::LastRelease tries to call the mutation observers for a LI element.  I haven't been able to track down the crash for now, but I thought I'd post the patch here to have roc's feedback on it.
Attachment #471967 - Flags: feedback?(roc)
I think instead we should probably back out bug 581585 and fix the performance issues in GetCaretFrameForNodeOffset another way. In retrospect, GetCaretFrameForNodeOffset is complicated and making sure we always detect a change in its inputs so we can flush the frame cache is going to be too fragile.

I think we could make nsBlockInFlowLineIterator::nsBlockInFlowLineIterator(nsBlockFrame* aFrame, nsIFrame* aFindFrame, PRBool* aFoundValidLine)
always start its search from the block's line cursor, if there is one. Lines are doubly-linked so you can search forward and backward at the same rate and find the line containing the frame in time proportional to the number of lines between the target line and the cursor. I think this would work well because the line cursor is the first line of the block that's visible. The caret is usually near there.
Attachment #471967 - Attachment is obsolete: true
Attachment #471967 - Flags: feedback?(roc)
Attachment #473211 - Flags: review?(roc)
Attachment #473212 - Flags: review?(roc)
Attached patch Part 3: Use the line cursor (obsolete) — Splinter Review
(In reply to comment #7)
> I think we could make
> nsBlockInFlowLineIterator::nsBlockInFlowLineIterator(nsBlockFrame* aFrame,
> nsIFrame* aFindFrame, PRBool* aFoundValidLine)
> always start its search from the block's line cursor, if there is one. Lines
> are doubly-linked so you can search forward and backward at the same rate and
> find the line containing the frame in time proportional to the number of lines
> between the target line and the cursor. I think this would work well because
> the line cursor is the first line of the block that's visible. The caret is
> usually near there.

Like this?
Attachment #473244 - Flags: review?(roc)
Keywords: regressionperf
Summary: Cursor lags text entry on Mozillazine Forums → Optimize caret movement without caching the caret frame and frame offset
+  nsIFrame *scrollFrame =
+    nsLayoutUtils::GetClosestFrameOfType(aFrame, nsGkAtoms::scrollFrame);
+  if (scrollFrame) {
+    nsIScrollableFrame *sf = do_QueryFrame(scrollFrame);
+    nsIFrame *scrolledFrame = sf->GetScrolledFrame();
+    nscoord offsetY = aFrame->GetOffsetTo(scrolledFrame).y;
+
+    // Don't use the line cursor if we might have a descendant placeholder ...
+    // it might skip lines that contain placeholders but don't themselves
+    // intersect with the dirty area.
+    nsLineBox* cursor = aFrame->GetStateBits() & NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO
+      ? nsnull : aFrame->GetFirstLineContaining(offsetY);
+    if (cursor) {

You don't need any of this. Just grab the LineCursorProperty() for the block and use it if it's there.

+  // If we reach here, it means that the cursor based search has failed,
+  // so we fall back to the dumb search strategy of starting from the first
+  // line and iterate over all of the lines linearly.
+
+  mLine = aFrame->begin_lines();

This means if the frame is in the overflow lines and not the in-flow lines, we will iterate over the main set of lines again. That's a bit wasteful.

I suggest we start with LineCursorProperty() and if that doesn't exist, start with mLines.begin(), and search all the in-flow lines the way you have it now. If that fails, instead of calling FindValidLine() just go ahead and start searching the overflow lines.
(In reply to comment #11)
> I suggest we start with LineCursorProperty() and if that doesn't exist, start
> with mLines.begin(), and search all the in-flow lines the way you have it now.
> If that fails, instead of calling FindValidLine() just go ahead and start
> searching the overflow lines.

Unless I'm missing something, this version of the iterator constructor will always set mInOverflowLines to null, which should means that it should never search the overflow lines.  Is that right?  If yes, then I think the old search code can be completely replaced with the new one.
FindValidLine() and Next() can set mInOverflowLines and search the overflow lines.
Right...
Attached patch Part 3: Use the line cursor (obsolete) — Splinter Review
In order to prevent code duplication, I decided to just set mLine to the end iterator and use the existing code to iterate over the overflow lines.  Hope I haven't missed anything...
Attachment #473244 - Attachment is obsolete: true
Attachment #474130 - Flags: review?(roc)
Attachment #473244 - Flags: review?(roc)
(In reply to comment #15)
> In order to prevent code duplication, I decided to just set mLine to the end
> iterator

You did?

+  mLine = aFrame->begin_lines();

Looks to me like you need to add mLine = aFrame->end_lines() at the end of the "if (cursor)" block, and set mLine to begin_lines if !cursor.
(In reply to comment #16)
> (In reply to comment #15)
> > In order to prevent code duplication, I decided to just set mLine to the end
> > iterator
> 
> You did?
> 
> +  mLine = aFrame->begin_lines();
> 
> Looks to me like you need to add mLine = aFrame->end_lines() at the end of the
> "if (cursor)" block,

Well, what I said was technically true, I _decided_ to do that, I just didn't do it! :-)

> and set mLine to begin_lines if !cursor.

Why is that needed?  I don't iterate over mLine if cursor is not null.
Attached patch Part 3: Use the line cursor (obsolete) — Splinter Review
Attachment #474130 - Attachment is obsolete: true
Attachment #474245 - Flags: review?(roc)
Attachment #474130 - Flags: review?(roc)
Attached patch Part 3: Use the line cursor (obsolete) — Splinter Review
Actually, I just realized that I had qrefreshed my changes on another patch in my mq, so the "begin_lines" version didn't include any of my changes, including the "end_lines" fix.  Turns out that I had actually made that change after all.
Attachment #474245 - Attachment is obsolete: true
Attachment #474250 - Flags: review?(roc)
Attachment #474245 - Flags: review?(roc)
Attached patch Part 3: Use the line cursor (obsolete) — Splinter Review
Arghhh.  Note to self: patch -p1 --dry-run is not really useful for fixing mistakes!
Attachment #474250 - Attachment is obsolete: true
Attachment #474251 - Flags: review?(roc)
Attachment #474250 - Flags: review?(roc)
Attached patch Rolled up patchSplinter Review
Attachment #474378 - Flags: approval2.0?
Attached patch Part 3: Use the line cursor (obsolete) — Splinter Review
Actually, the frame's line iterator could be invalid when we try to dereference it, which would abort the program in debug builds.  This version of the patch checks the iterator before setting cursor to its current value to make sure that doesn't happen.
Attachment #474251 - Attachment is obsolete: true
Attachment #474464 - Flags: review?(roc)
Attachment #474464 - Flags: approval2.0?
Attached patch Part 3: Use the line cursor (obsolete) — Splinter Review
One further correction: we shouldn't return at the end of the for loop iterating over |line| and |rline|, we should fall back to searching the overflow lines, like this:

--- a/layout/generic/nsBlockFrame.cpp
+++ b/layout/generic/nsBlockFrame.cpp
@@ -5092,18 +5092,17 @@ nsBlockInFlowLineIterator::nsBlockInFlow
     // line cursor.
     nsBlockFrame::line_iterator line = aFrame->line(cursor);
     nsBlockFrame::reverse_line_iterator rline = aFrame->rline(cursor);
     nsBlockFrame::line_iterator line_end = aFrame->end_lines();
     nsBlockFrame::reverse_line_iterator rline_end = aFrame->rend_lines();
     for (--rline;;) {
       if (line == line_end && rline == rline_end) {
         // Didn't find the line
-        mLine = line;
-        return;
+        break;
       }
       if (line != line_end) {
         if (line->Contains(child)) {
           *aFoundValidLine = PR_TRUE;
           mLine = line;
           return;
         }
         ++line;

This version of the patch adds the above change.
Attachment #474464 - Attachment is obsolete: true
Attachment #474542 - Flags: review?(roc)
Attachment #474542 - Flags: approval2.0?
Attachment #474464 - Flags: review?(roc)
Attachment #474464 - Flags: approval2.0?
No it doesn't! but r=me with that change.
This time with the actual change included in the patch...
Attachment #474542 - Attachment is obsolete: true
Attachment #474707 - Flags: review?(roc)
Attachment #474707 - Flags: approval2.0?
Attachment #474542 - Flags: review?(roc)
Attachment #474542 - Flags: approval2.0?
Attachment #474707 - Flags: review?(roc)
Attachment #474707 - Flags: review+
Attachment #474707 - Flags: approval2.0?
Attachment #474707 - Flags: approval2.0+
http://hg.mozilla.org/mozilla-central/rev/1e0a9cf6dc53
http://hg.mozilla.org/mozilla-central/rev/251816ad0480
http://hg.mozilla.org/mozilla-central/rev/0f5a2dce1aa5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Depends on: 596710
Depends on: 597331
No longer depends on: 597331
You need to log in before you can comment on or make changes to this bug.