Changeset 212151 in webkit

Timestamp:

Feb 10, 2017 2:36:12 PM (7 years ago)

Author:

Alan Bujtas

Message:

Mail hangs when removing multiple rows from large table.
​https://bugs.webkit.org/show_bug.cgi?id=168103
<rdar://problem/30090186>

Reviewed by Ryosuke Niwa.

PerformanceTests:

• DOM/large-table-edit.html: Added.

Source/WebCore:

DeleteSelectionCommand::removeNode doesn't actually destroy table structure items,
but instead it removes their content. In order to be able to continue editing the table after
the delete, we need to ensure that its cells' width and height are > 0. Currently we issue layout on
each table item recursively.
This patch delays the layout until after we've finished with the entire subtree delete (10x progression).

Performance test added.

• editing/DeleteSelectionCommand.cpp:

(WebCore::DeleteSelectionCommand::insertBlockPlaceholderForTableCellIfNeeded):
(WebCore::DeleteSelectionCommand::removeNodeUpdatingStates):
(WebCore::shouldRemoveContentOnly):
(WebCore::DeleteSelectionCommand::removeNode):

• editing/DeleteSelectionCommand.h:

Location:

trunk

Files:

• PerformanceTests/ChangeLog (modified) (1 diff)
• PerformanceTests/DOM/large-table-edit.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/editing/DeleteSelectionCommand.cpp (modified) (3 diffs)
• Source/WebCore/editing/DeleteSelectionCommand.h (modified) (1 diff)

trunk/PerformanceTests/ChangeLog

@@ +1 @@
+2017-02-10  Zalan Bujtas  <zalan@apple.com>
+
+        Mail hangs when removing multiple rows from large table.
+        https://bugs.webkit.org/show_bug.cgi?id=168103
+        <rdar://problem/30090186>
+
+        Reviewed by Ryosuke Niwa.
+
+        * DOM/large-table-edit.html: Added.
+
 2017-02-05  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-02-10  Zalan Bujtas  <zalan@apple.com>
+
+        Mail hangs when removing multiple rows from large table.
+        https://bugs.webkit.org/show_bug.cgi?id=168103
+        <rdar://problem/30090186>
+
+        Reviewed by Ryosuke Niwa.
+
+        DeleteSelectionCommand::removeNode doesn't actually destroy table structure items,
+        but instead it removes their content. In order to be able to continue editing the table after
+        the delete, we need to ensure that its cells' width and height are > 0. Currently we issue layout on
+        each table item recursively.
+        This patch delays the layout until after we've finished with the entire subtree delete (10x progression).
+
+        Performance test added.
+
+        * editing/DeleteSelectionCommand.cpp:
+        (WebCore::DeleteSelectionCommand::insertBlockPlaceholderForTableCellIfNeeded):
+        (WebCore::DeleteSelectionCommand::removeNodeUpdatingStates):
+        (WebCore::shouldRemoveContentOnly):
+        (WebCore::DeleteSelectionCommand::removeNode):
+        * editing/DeleteSelectionCommand.h:
+
 2017-02-10  Joseph Pecoraro  <pecoraro@apple.com>
 

trunk/Source/WebCore/editing/DeleteSelectionCommand.cpp

@@ -31 +31 @@
 #include "Editor.h"
 #include "EditorClient.h"
+#include "ElementIterator.h"
 #include "Frame.h"
 #include "HTMLBRElement.h"
@@ -343 +344 @@
         next = NodeTraversal::next(*next, node);
     return next ? firstPositionInOrBeforeNode(next) : Position();
+}
+
+void DeleteSelectionCommand::insertBlockPlaceholderForTableCellIfNeeded(Element& element)
+{
+    // Make sure empty cell has some height.
+    auto* renderer = element.renderer();
+    if (!is<RenderTableCell>(renderer))
+        return;
+    if (downcast<RenderTableCell>(*renderer).contentHeight() > 0)
+        return;
+    insertBlockPlaceholder(firstEditablePositionInNode(&element));
+}
+    
+void DeleteSelectionCommand::removeNodeUpdatingStates(Node& node, ShouldAssumeContentIsAlwaysEditable shouldAssumeContentIsAlwaysEditable)
+{
+    if (&node == m_startBlock && !isEndOfBlock(VisiblePosition(firstPositionInNode(m_startBlock.get())).previous()))
+        m_needPlaceholder = true;
+    else if (&node == m_endBlock && !isStartOfBlock(VisiblePosition(lastPositionInNode(m_startBlock.get())).next()))
+        m_needPlaceholder = true;
+    
+    // FIXME: Update the endpoints of the range being deleted.
+    updatePositionForNodeRemoval(m_endingPosition, node);
+    updatePositionForNodeRemoval(m_leadingWhitespace, node);
+    updatePositionForNodeRemoval(m_trailingWhitespace, node);
+    
+    CompositeEditCommand::removeNode(&node, shouldAssumeContentIsAlwaysEditable);
+}
+    
+static inline bool shouldRemoveContentOnly(const Node& node)
+{
+    return isTableStructureNode(&node) || node.isRootEditableElement();
 }
 
@@ -372 +404 @@
     }
     
-    if (isTableStructureNode(node.get()) || node->isRootEditableElement()) {
+    if (shouldRemoveContentOnly(*node)) {
         // Do not remove an element of table structure; remove its contents.
         // Likewise for the root editable element.
-        Node* child = node->firstChild();
+        auto* child = NodeTraversal::next(*node, node.get());
         while (child) {
-            Node* remove = child;
-            child = child->nextSibling();
-            removeNode(remove, shouldAssumeContentIsAlwaysEditable);
-        }
-        
-        // Make sure empty cell has some height, if a placeholder can be inserted.
+            if (shouldRemoveContentOnly(*child)) {
+                child = NodeTraversal::next(*child, node.get());
+                continue;
+            }
+            auto* remove = child;
+            child = NodeTraversal::nextSkippingChildren(*child, node.get());
+            removeNodeUpdatingStates(*remove, shouldAssumeContentIsAlwaysEditable);
+        }
+        
+        ASSERT(is<Element>(*node));
+        auto& element = downcast<Element>(*node);
         document().updateLayoutIgnorePendingStylesheets();
-        RenderObject* renderer = node->renderer();
-        if (is<RenderTableCell>(renderer) && downcast<RenderTableCell>(*renderer).contentHeight() <= 0) {
-            Position firstEditablePosition = firstEditablePositionInNode(node.get());
-            if (firstEditablePosition.isNotNull())
-                insertBlockPlaceholder(firstEditablePosition);
-        }
-        return;
-    }
-    
-    if (node == m_startBlock && !isEndOfBlock(VisiblePosition(firstPositionInNode(m_startBlock.get())).previous()))
-        m_needPlaceholder = true;
-    else if (node == m_endBlock && !isStartOfBlock(VisiblePosition(lastPositionInNode(m_startBlock.get())).next()))
-        m_needPlaceholder = true;
-    
-    // FIXME: Update the endpoints of the range being deleted.
-    updatePositionForNodeRemoval(m_endingPosition, *node);
-    updatePositionForNodeRemoval(m_leadingWhitespace, *node);
-    updatePositionForNodeRemoval(m_trailingWhitespace, *node);
-    
-    CompositeEditCommand::removeNode(node, shouldAssumeContentIsAlwaysEditable);
+        // Check if we need to insert a placeholder for descendant table cells.
+        auto* descendant = ElementTraversal::next(element, &element);
+        while (descendant) {
+            auto* placeholderCandidate = descendant;
+            descendant = ElementTraversal::next(*descendant, &element);
+            insertBlockPlaceholderForTableCellIfNeeded(*placeholderCandidate);
+        }
+        insertBlockPlaceholderForTableCellIfNeeded(element);
+        return;
+    }
+    removeNodeUpdatingStates(*node, shouldAssumeContentIsAlwaysEditable);
 }
 

trunk/Source/WebCore/editing/DeleteSelectionCommand.h

@@ -74 +74 @@
     String originalStringForAutocorrectionAtBeginningOfSelection();
 
+    void removeNodeUpdatingStates(Node&, ShouldAssumeContentIsAlwaysEditable);
+    void insertBlockPlaceholderForTableCellIfNeeded(Element&);
+
     bool m_hasSelectionToDelete;
     bool m_smartDelete;