Bug 83983 - REGRESSION(r96257): Deleting a large amount of text is very slow
Summary: REGRESSION(r96257): Deleting a large amount of text is very slow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-04-14 11:56 PDT by Adele Peterson
Modified: 2012-04-27 11:50 PDT (History)
5 users (show)

See Also:


Attachments
test (1.37 MB, text/html)
2012-04-14 11:56 PDT, Adele Peterson
no flags Details
Patch (4.68 KB, patch)
2012-04-26 17:58 PDT, Enrica Casucci
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.