WebCore:
authorjustin.garcia@apple.com <justin.garcia@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Feb 2008 07:17:23 +0000 (07:17 +0000)
committerjustin.garcia@apple.com <justin.garcia@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Feb 2008 07:17:23 +0000 (07:17 +0000)
        Reviewed by Darin Adler.

        <rdar://problem/5195056> Huge plain text pastes are slow

        This was fixed in r27369 and then r29367 and r29667 caused performance to
        regress.

        * editing/EditCommand.cpp:
        (WebCore::EditCommand::apply): Only updateLayout() for high level commands.
        (WebCore::EditCommand::unapply): Ditto.
        (WebCore::EditCommand::reapply): Ditto.
        * editing/Editor.cpp:
        (WebCore::Editor::appliedEditing): Added a note about shouldChangeSelection calls
        that shouldn't be made, a bug I filed as <rdar://problem/5729315>.
        (WebCore::Editor::unappliedEditing): Ditto.
        (WebCore::Editor::reappliedEditing): Ditto.
        * editing/SelectionController.cpp:
        (WebCore::SelectionController::nodeWillBeRemoved): Don't try to test the selection
        base and extent with the expensive isCandidate operation if the node that will
        be removed is in a fragment, since such a removal is guaranteed to have no effect
        on a selection.  This is to speed up the paste operation, which does many removes from
        a fragment.

LayoutTests:

        Reviewed by Darin Adler.

        <rdar://problem/5195056> Huge plain text pastes are slow

        The changes made for this fix exposed several more cases of:
        <rdar://problem/5729315> Some shouldChangeSelectedDOMRange contain Ranges for selections that are no longer valid

        * platform/mac/editing/deleting/collapse-whitespace-3587601-fix-expected.txt:
        * platform/mac/editing/deleting/delete-3608462-fix-expected.txt:
        * platform/mac/editing/deleting/delete-4083333-fix-expected.txt:
        * platform/mac/editing/execCommand/find-after-replace-expected.txt:
        * platform/mac/editing/selection/move-between-blocks-no-001-expected.txt:
        * platform/mac/editing/selection/replace-selection-1-expected.txt:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@30062 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/platform/mac/editing/deleting/collapse-whitespace-3587601-fix-expected.txt
LayoutTests/platform/mac/editing/deleting/delete-3608462-fix-expected.txt
LayoutTests/platform/mac/editing/deleting/delete-4083333-fix-expected.txt
LayoutTests/platform/mac/editing/execCommand/find-after-replace-expected.txt
LayoutTests/platform/mac/editing/selection/move-between-blocks-no-001-expected.txt
LayoutTests/platform/mac/editing/selection/replace-selection-1-expected.txt
WebCore/ChangeLog
WebCore/editing/EditCommand.cpp
WebCore/editing/Editor.cpp
WebCore/editing/SelectionController.cpp

index fba082f..4c27a62 100644 (file)
@@ -1,3 +1,19 @@
+2008-02-06  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by Darin Adler.
+        
+        <rdar://problem/5195056> Huge plain text pastes are slow
+        
+        The changes made for this fix exposed several more cases of:
+        <rdar://problem/5729315> Some shouldChangeSelectedDOMRange contain Ranges for selections that are no longer valid
+
+        * platform/mac/editing/deleting/collapse-whitespace-3587601-fix-expected.txt:
+        * platform/mac/editing/deleting/delete-3608462-fix-expected.txt:
+        * platform/mac/editing/deleting/delete-4083333-fix-expected.txt:
+        * platform/mac/editing/execCommand/find-after-replace-expected.txt:
+        * platform/mac/editing/selection/move-between-blocks-no-001-expected.txt:
+        * platform/mac/editing/selection/replace-selection-1-expected.txt:
+
 2008-02-06  Alexey Proskuryakov  <ap@webkit.org>
 
         Landing updated (improved) results for this test. The results changed in r30013,
index f4f25c7..03b9c5c 100644 (file)
@@ -11,6 +11,8 @@ EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotificatio
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: shouldDeleteDOMRange:range from 4 of #text > SPAN > DIV > BODY > HTML > #document to 5 of #text > SPAN > DIV > BODY > HTML > #document
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 4 of #text > SPAN > DIV > BODY > HTML > #document to 4 of #text > SPAN > DIV > BODY > HTML > #document toDOMRange:range from 4 of #text > SPAN > DIV > BODY > HTML > #document to 4 of #text > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
 EDITING DELEGATE: shouldDeleteDOMRange:range from 3 of #text > SPAN > DIV > BODY > HTML > #document to 4 of #text > SPAN > DIV > BODY > HTML > #document
 EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 3 of #text > SPAN > DIV > BODY > HTML > #document to 3 of #text > SPAN > DIV > BODY > HTML > #document toDOMRange:range from 3 of #text > SPAN > DIV > BODY > HTML > #document to 3 of #text > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
index 9a4484f..0c8e235 100644 (file)
@@ -13,6 +13,8 @@ EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotificatio
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: shouldDeleteDOMRange:range from 3 of #text > SPAN > DIV > BODY > HTML > #document to 4 of #text > SPAN > DIV > BODY > HTML > #document
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 3 of #text > SPAN > DIV > BODY > HTML > #document to 3 of #text > SPAN > DIV > BODY > HTML > #document toDOMRange:range from 3 of #text > SPAN > DIV > BODY > HTML > #document to 3 of #text > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
 EDITING DELEGATE: shouldDeleteDOMRange:range from 2 of #text > SPAN > DIV > BODY > HTML > #document to 3 of #text > SPAN > DIV > BODY > HTML > #document
 EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 2 of #text > SPAN > DIV > BODY > HTML > #document to 2 of #text > SPAN > DIV > BODY > HTML > #document toDOMRange:range from 2 of #text > SPAN > DIV > BODY > HTML > #document to 2 of #text > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
index d1e39c1..28edcf0 100644 (file)
@@ -3,6 +3,8 @@ EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: shouldDeleteDOMRange:range from 3 of #text > A > SPAN > DIV > BODY > HTML > #document to 4 of #text > A > SPAN > DIV > BODY > HTML > #document
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 3 of #text > A > SPAN > DIV > BODY > HTML > #document to 3 of #text > A > SPAN > DIV > BODY > HTML > #document toDOMRange:range from 3 of #text > A > SPAN > DIV > BODY > HTML > #document to 3 of #text > A > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
 layer at (0,0) size 800x600
   RenderView at (0,0) size 800x600
index dd21930..ce2bfa3 100644 (file)
@@ -6,7 +6,7 @@ EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotificatio
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: shouldInsertNode:#document-fragment replacingDOMRange:range from 4 of #text > BODY > HTML > #document to 5 of #text > BODY > HTML > #document givenAction:WebViewInsertActionPasted
-EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 4 of #text > BODY > HTML > #document to 4 of #text > BODY > HTML > #document toDOMRange:range from 3 of #text > BODY > HTML > #document to 3 of #text > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 4 of #text > BODY > HTML > #document to 0 of #text > BODY > HTML > #document toDOMRange:range from 3 of #text > BODY > HTML > #document to 3 of #text > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
 layer at (0,0) size 800x600
index fa888b1..c542e2a 100644 (file)
@@ -42,6 +42,8 @@ EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotificatio
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: shouldDeleteDOMRange:range from 39 of #text > SPAN > DIV > BODY > HTML > #document to 40 of #text > SPAN > DIV > BODY > HTML > #document
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 39 of #text > SPAN > DIV > BODY > HTML > #document to 39 of #text > SPAN > DIV > BODY > HTML > #document toDOMRange:range from 39 of #text > SPAN > DIV > BODY > HTML > #document to 39 of #text > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
 EDITING DELEGATE: shouldDeleteDOMRange:range from 38 of #text > SPAN > DIV > BODY > HTML > #document to 39 of #text > SPAN > DIV > BODY > HTML > #document
 EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 38 of #text > SPAN > DIV > BODY > HTML > #document to 38 of #text > SPAN > DIV > BODY > HTML > #document toDOMRange:range from 38 of #text > SPAN > DIV > BODY > HTML > #document to 38 of #text > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
index 5dcd021..af4e5aa 100644 (file)
@@ -1,7 +1,7 @@
 EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of DIV > BODY > HTML > #document to 1 of DIV > BODY > HTML > #document
 EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
-EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 6 of #text > DIV > BODY > HTML > #document to 6 of #text > DIV > BODY > HTML > #document toDOMRange:range from 4 of #text > A > DIV > BODY > HTML > #document to 4 of #text > A > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 6 of #text > DIV > BODY > HTML > #document to 0 of #text > A > DIV > BODY > HTML > #document toDOMRange:range from 4 of #text > A > DIV > BODY > HTML > #document to 4 of #text > A > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
index 5175e5f..0d6ea65 100644 (file)
@@ -1,3 +1,28 @@
+2008-02-06  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by Darin Adler.
+
+        <rdar://problem/5195056> Huge plain text pastes are slow
+        
+        This was fixed in r27369 and then r29367 and r29667 caused performance to
+        regress.
+
+        * editing/EditCommand.cpp:
+        (WebCore::EditCommand::apply): Only updateLayout() for high level commands.
+        (WebCore::EditCommand::unapply): Ditto.
+        (WebCore::EditCommand::reapply): Ditto.
+        * editing/Editor.cpp:
+        (WebCore::Editor::appliedEditing): Added a note about shouldChangeSelection calls 
+        that shouldn't be made, a bug I filed as <rdar://problem/5729315>.
+        (WebCore::Editor::unappliedEditing): Ditto.
+        (WebCore::Editor::reappliedEditing): Ditto.
+        * editing/SelectionController.cpp:
+        (WebCore::SelectionController::nodeWillBeRemoved): Don't try to test the selection
+        base and extent with the expensive isCandidate operation if the node that will
+        be removed is in a fragment, since such a removal is guaranteed to have no effect
+        on a selection.  This is to speed up the paste operation, which does many removes from
+        a fragment.
+
 2008-02-06  Kevin Ollivier  <kevino@theolliviers.com>
 
         Reviewed by Darin Adler.
index b6b27cf..7a5d752 100644 (file)
@@ -82,7 +82,12 @@ void EditCommand::apply()
         }
     }
     
-    updateLayout();
+    // Changes to the document may have been made since the last editing operation that 
+    // require a layout, as in <rdar://problem/5658603>.  Low level operations, like 
+    // RemoveNodeCommand, don't require a layout because the high level operations that 
+    // use them perform one if one is necessary (like for the creation of VisiblePositions).
+    if (!m_parent)
+        updateLayout();
 
     DeleteButtonController* deleteButtonController = frame->editor()->deleteButtonController();
     deleteButtonController->disable();
@@ -111,8 +116,11 @@ void EditCommand::unapply()
     Frame* frame = m_document->frame();
     
     // Changes to the document may have been made since the last editing operation that 
-    // require a layout, as in <rdar://problem/5658603>.
-    updateLayout();
+    // require a layout, as in <rdar://problem/5658603>.  Low level operations, like 
+    // RemoveNodeCommand, don't require a layout because the high level operations that 
+    // use them perform one if one is necessary (like for the creation of VisiblePositions).
+    if (!m_parent)
+        updateLayout();
     
     DeleteButtonController* deleteButtonController = frame->editor()->deleteButtonController();
     deleteButtonController->disable();
@@ -133,8 +141,11 @@ void EditCommand::reapply()
     Frame* frame = m_document->frame();
     
     // Changes to the document may have been made since the last editing operation that 
-    // require a layout, as in <rdar://problem/5658603>.
-    updateLayout();
+    // require a layout, as in <rdar://problem/5658603>.  Low level operations, like 
+    // RemoveNodeCommand, don't require a layout because the high level operations that 
+    // use them perform one if one is necessary (like for the creation of VisiblePositions).
+    if (!m_parent)
+        updateLayout();
 
     DeleteButtonController* deleteButtonController = frame->editor()->deleteButtonController();
     deleteButtonController->disable();
index cd656cc..3f4faa4 100644 (file)
@@ -762,6 +762,8 @@ void Editor::appliedEditing(PassRefPtr<EditCommand> cmd)
     Selection newSelection(cmd->endingSelection());
     // If there is no selection change, don't bother sending shouldChangeSelection, but still call setSelection,
     // because there is work that it must do in this situation.
+    // The old selection can be invalid here and calling shouldChangeSelection can produce some strange calls.
+    // See <rdar://problem/5729315> Some shouldChangeSelectedDOMRange contain Ranges for selections that are no longer valid
     if (newSelection == m_frame->selectionController()->selection() || m_frame->shouldChangeSelection(newSelection))
         m_frame->selectionController()->setSelection(newSelection, false);
     
@@ -798,6 +800,8 @@ void Editor::unappliedEditing(PassRefPtr<EditCommand> cmd)
     Selection newSelection(cmd->startingSelection());
     // If there is no selection change, don't bother sending shouldChangeSelection, but still call setSelection,
     // because there is work that it must do in this situation.
+    // The old selection can be invalid here and calling shouldChangeSelection can produce some strange calls.
+    // See <rdar://problem/5729315> Some shouldChangeSelectedDOMRange contain Ranges for selections that are no longer valid
     if (newSelection == m_frame->selectionController()->selection() || m_frame->shouldChangeSelection(newSelection))
         m_frame->selectionController()->setSelection(newSelection, true);
     
@@ -814,6 +818,8 @@ void Editor::reappliedEditing(PassRefPtr<EditCommand> cmd)
     Selection newSelection(cmd->endingSelection());
     // If there is no selection change, don't bother sending shouldChangeSelection, but still call setSelection,
     // because there is work that it must do in this situation.
+    // The old selection can be invalid here and calling shouldChangeSelection can produce some strange calls.
+    // See <rdar://problem/5729315> Some shouldChangeSelectedDOMRange contain Ranges for selections that are no longer valid
     if (newSelection == m_frame->selectionController()->selection() || m_frame->shouldChangeSelection(newSelection))
         m_frame->selectionController()->setSelection(newSelection, true);
     
index 880876a..5820c81 100644 (file)
@@ -167,6 +167,11 @@ void SelectionController::nodeWillBeRemoved(Node *node)
 {
     if (isNone())
         return;
+        
+    // There can't be a selection inside a fragment, so if a fragment's node is being removed,
+    // the selection in the document that created the fragment needs no adjustment.
+    if (node && highestAncestor(node)->nodeType() == Node::DOCUMENT_FRAGMENT_NODE)
+        return;
     
     bool baseRemoved = !m_sel.base().isCandidate() || removingNodeRemovesPosition(node, m_sel.base());
     bool extentRemoved = !m_sel.extent().isCandidate() || removingNodeRemovesPosition(node, m_sel.extent());