Bug 83983

Summary: REGRESSION(r96257): Deleting a large amount of text is very slow
Product: WebKit Reporter: Adele Peterson <adele>
Component: HTML EditingAssignee: Enrica Casucci <enrica>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, enrica, mitz, rniwa, una.sabovic
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
test
none
Patch rniwa: review+

Description Adele Peterson 2012-04-14 11:56:59 PDT
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>
Comment 1 Enrica Casucci 2012-04-26 17:58:10 PDT
Created attachment 139113 [details]
Patch
Comment 2 Ryosuke Niwa 2012-04-26 18:08:42 PDT
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.
Comment 3 Ryosuke Niwa 2012-04-26 18:09:39 PDT
clip, trim, or truncate may also work.
Comment 4 Enrica Casucci 2012-04-26 22:26:32 PDT
> > 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.
Comment 5 Ryosuke Niwa 2012-04-26 22:32:38 PDT
(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.
Comment 6 Enrica Casucci 2012-04-27 11:50:42 PDT
Committed revision 115460.