Bug 83983 – REGRESSION(r96257): Deleting a large amount of text is very slow

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Adele Peterson]

Created attachment 137218 [details]
test

Regressed with the change for:

Setting innerText to an empty string on editable div loses focus
 https://bugs.webkit.org/show_bug.cgi?id=62092
http://trac.webkit.org/changeset/96257

Load the attached test case, select-all, and delete.  Before this change it was reasonably quick.  After this change, there's a very long wait (not actually sure if it ever finishes).

The markup in the test is very simple - just lots of <br>s and text.

<rdar://problem/10826076>

[Enrica Casucci]

Created attachment 139113 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 139113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139113&action=review

Thanks for the fix!

> Source/WebCore/ChangeLog:17
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description.

> Source/WebCore/dom/Position.cpp:180
> +        return minOffsetForNode(m_anchorNode.get(), m_offset);

minOffset sounds like it's the minimum offset. Maybe capOffsetInNode? or roundOffsetInNode?

> Source/WebCore/dom/Position.h:295
> +    for (Node* n = node->firstChild(); n && newOffset < offset; n = n->nextSibling())

Please don't use abbreviations like n.

> Source/WebCore/dom/Position.h:307
> +    for (Node* n = node->firstChild(); n && currentOffset < offset; n = n->nextSibling())

Ditto.

[Ryosuke Niwa]

clip, trim, or truncate may also work.

[Enrica Casucci]

> > Source/WebCore/dom/Position.cpp:180
> > +        return minOffsetForNode(m_anchorNode.get(), m_offset);
> 
> minOffset sounds like it's the minimum offset. Maybe capOffsetInNode? or roundOffsetInNode?
I'm not sure I agree. This is actually the minimum between offset and the last offset in the node).

> 
> > Source/WebCore/dom/Position.h:295
> > +    for (Node* n = node->firstChild(); n && newOffset < offset; n = n->nextSibling())
> 
> Please don't use abbreviations like n.
> 
> > Source/WebCore/dom/Position.h:307
> > +    for (Node* n = node->firstChild(); n && currentOffset < offset; n = n->nextSibling())
> 
> Ditto.

You're right, I'll fix it.
Thanks for the review.

[Ryosuke Niwa]

(In reply to comment #4)
> > > Source/WebCore/dom/Position.cpp:180
> > > +        return minOffsetForNode(m_anchorNode.get(), m_offset);
> > 
> > minOffset sounds like it's the minimum offset. Maybe capOffsetInNode? or roundOffsetInNode?
> I'm not sure I agree. This is actually the minimum between offset and the last offset in the node).

Okay. I guess it's not as counter-intutive on my second thought.

[Enrica Casucci]

Committed revision 115460.