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>
Created attachment 139113 [details] Patch
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.
clip, trim, or truncate may also work.
> > 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.
(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.
Committed revision 115460.