Mail hangs when resizing the font size of a large RTL text
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 11 Dec 2012 23:33:49 +0000 (23:33 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 11 Dec 2012 23:33:49 +0000 (23:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=104643

Reviewed by Enrica Casucci.

The bug was caused by ApplyStyleCommand::applyInlineStyleToNodeRange obtaining computed styles while
removing and adding nodes. Fixed the slowness by breaking it into three phases:
1. Split the range into contiguous inline runs, and determine whether styles need to be removed or applied.
2. Remove any conflicting styles, and insert dummy elements at positions where inline styles ought to be
computed as needed.
3. Remove the dummy elements and apply ilnine styles as needed.

No new tests are added since there is no behavior change. This is just a performance improvement.

* editing/ApplyStyleCommand.cpp:
(WebCore::InlineRunToApplyStyle::InlineRunToApplyStyle):
(WebCore::InlineRunToApplyStyle::startAndEndAreStillInDocument):
(WebCore::ApplyStyleCommand::applyInlineStyleToNodeRange):
(WebCore::ApplyStyleCommand::shouldApplyInlineStyleToRun):
(WebCore::ApplyStyleCommand::removeConflictingInlineStyleFromRun):
(WebCore::ApplyStyleCommand::addInlineStyleIfNeeded):
(WebCore::ApplyStyleCommand::positionToComputeInlineStyleChange):
(WebCore::ApplyStyleCommand::applyInlineStyleChange):
* editing/ApplyStyleCommand.h:
* editing/EditingStyle.h:
(WebCore::StyleChange::StyleChange):

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

Source/WebCore/ChangeLog
Source/WebCore/editing/ApplyStyleCommand.cpp
Source/WebCore/editing/ApplyStyleCommand.h
Source/WebCore/editing/EditingStyle.h

index e81fb36..39dc34a 100644 (file)
@@ -1,3 +1,32 @@
+2012-12-11  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Mail hangs when resizing the font size of a large RTL text
+        https://bugs.webkit.org/show_bug.cgi?id=104643
+
+        Reviewed by Enrica Casucci.
+
+        The bug was caused by ApplyStyleCommand::applyInlineStyleToNodeRange obtaining computed styles while
+        removing and adding nodes. Fixed the slowness by breaking it into three phases:
+        1. Split the range into contiguous inline runs, and determine whether styles need to be removed or applied.
+        2. Remove any conflicting styles, and insert dummy elements at positions where inline styles ought to be
+        computed as needed.
+        3. Remove the dummy elements and apply ilnine styles as needed.
+
+        No new tests are added since there is no behavior change. This is just a performance improvement. 
+
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::InlineRunToApplyStyle::InlineRunToApplyStyle):
+        (WebCore::InlineRunToApplyStyle::startAndEndAreStillInDocument):
+        (WebCore::ApplyStyleCommand::applyInlineStyleToNodeRange):
+        (WebCore::ApplyStyleCommand::shouldApplyInlineStyleToRun):
+        (WebCore::ApplyStyleCommand::removeConflictingInlineStyleFromRun):
+        (WebCore::ApplyStyleCommand::addInlineStyleIfNeeded):
+        (WebCore::ApplyStyleCommand::positionToComputeInlineStyleChange):
+        (WebCore::ApplyStyleCommand::applyInlineStyleChange):
+        * editing/ApplyStyleCommand.h:
+        * editing/EditingStyle.h:
+        (WebCore::StyleChange::StyleChange):
+
 2012-12-11  Xianzhu Wang  <wangxianzhu@chromium.org>
 
         Incorrect position of layers for fixed position elements when page is scaled smaller than viewport
index bfee1ba..ce471e2 100644 (file)
@@ -716,11 +716,36 @@ static bool containsNonEditableRegion(Node* node)
     return false;
 }
 
+struct InlineRunToApplyStyle {
+    InlineRunToApplyStyle(Node* start, Node* end, Node* pastEndNode)
+        : start(start)
+        , end(end)
+        , pastEndNode(pastEndNode)
+    {
+        ASSERT(start->parentNode() == end->parentNode());
+    }
+
+    bool startAndEndAreStillInDocument()
+    {
+        return start && end && start->inDocument() && end->inDocument();
+    }
+
+    RefPtr<Node> start;
+    RefPtr<Node> end;
+    RefPtr<Node> pastEndNode;
+    Position positionForStyleComputation;
+    RefPtr<Node> dummyElement;
+    StyleChange change;
+};
+
 void ApplyStyleCommand::applyInlineStyleToNodeRange(EditingStyle* style, PassRefPtr<Node> startNode, PassRefPtr<Node> pastEndNode)
 {
     if (m_removeOnly)
         return;
 
+    document()->updateLayoutIgnorePendingStylesheets();
+
+    Vector<InlineRunToApplyStyle> runs;
     RefPtr<Node> node = startNode;
     for (RefPtr<Node> next; node && node != pastEndNode; node = next) {
         next = NodeTraversal::next(node.get());
@@ -755,8 +780,8 @@ void ApplyStyleCommand::applyInlineStyleToNodeRange(EditingStyle* style, PassRef
             }
         }
 
-        RefPtr<Node> runStart = node;
-        RefPtr<Node> runEnd = node;
+        Node* runStart = node.get();
+        Node* runEnd = node.get();
         Node* sibling = node->nextSibling();
         while (sibling && sibling != pastEndNode && !sibling->contains(pastEndNode.get())
                && (!isBlock(sibling) || sibling->hasTagName(brTag))
@@ -764,11 +789,31 @@ void ApplyStyleCommand::applyInlineStyleToNodeRange(EditingStyle* style, PassRef
             runEnd = sibling;
             sibling = runEnd->nextSibling();
         }
-        next = NodeTraversal::nextSkippingChildren(runEnd.get());
+        next = NodeTraversal::nextSkippingChildren(runEnd);
 
-        if (!removeStyleFromRunBeforeApplyingStyle(style, runStart, runEnd))
+        Node* pastEndNode = NodeTraversal::nextSkippingChildren(runEnd);
+        if (!shouldApplyInlineStyleToRun(style, runStart, pastEndNode))
             continue;
-        addInlineStyleIfNeeded(style, runStart.get(), runEnd.get(), AddStyledElement);
+
+        runs.append(InlineRunToApplyStyle(runStart, runEnd, pastEndNode));
+    }
+
+    for (size_t i = 0; i < runs.size(); i++) {
+        removeConflictingInlineStyleFromRun(style, runs[i].start, runs[i].end, runs[i].pastEndNode);
+        runs[i].positionForStyleComputation = positionToComputeInlineStyleChange(runs[i].start, runs[i].dummyElement);
+    }
+
+    document()->updateLayoutIgnorePendingStylesheets();
+
+    for (size_t i = 0; i < runs.size(); i++)
+        runs[i].change = StyleChange(style, runs[i].positionForStyleComputation);
+
+    for (size_t i = 0; i < runs.size(); i++) {
+        InlineRunToApplyStyle& run = runs[i];
+        if (run.dummyElement)
+            removeNode(run.dummyElement);
+        if (run.startAndEndAreStillInDocument())
+            applyInlineStyleChange(run.start.release(), run.end.release(), run.change, AddStyledElement);
     }
 }
 
@@ -778,24 +823,25 @@ bool ApplyStyleCommand::isStyledInlineElementToRemove(Element* element) const
         || (m_isInlineElementToRemoveFunction && m_isInlineElementToRemoveFunction(element));
 }
 
-bool ApplyStyleCommand::removeStyleFromRunBeforeApplyingStyle(EditingStyle* style, RefPtr<Node>& runStart, RefPtr<Node>& runEnd)
+bool ApplyStyleCommand::shouldApplyInlineStyleToRun(EditingStyle* style, Node* runStart, Node* pastEndNode)
 {
-    ASSERT(runStart && runEnd && runStart->parentNode() == runEnd->parentNode());
-    RefPtr<Node> pastEndNode = NodeTraversal::nextSkippingChildren(runEnd.get());
-    bool needToApplyStyle = false;
-    for (Node* node = runStart.get(); node && node != pastEndNode.get(); node = NodeTraversal::next(node)) {
+    ASSERT(style && runStart);
+
+    for (Node* node = runStart; node && node != pastEndNode; node = NodeTraversal::next(node)) {
         if (node->childNodeCount())
             continue;
         // We don't consider m_isInlineElementToRemoveFunction here because we never apply style when m_isInlineElementToRemoveFunction is specified
-        if (!style->styleIsPresentInComputedStyleOfNode(node)
-            || (m_styledInlineElement && !enclosingNodeWithTag(positionBeforeNode(node), m_styledInlineElement->tagQName()))) {
-            needToApplyStyle = true;
-            break;
-        }
+        if (!style->styleIsPresentInComputedStyleOfNode(node))
+            return true;
+        if (m_styledInlineElement && !enclosingNodeWithTag(positionBeforeNode(node), m_styledInlineElement->tagQName()))
+            return true;
     }
-    if (!needToApplyStyle)
-        return false;
+    return false;
+}
 
+void ApplyStyleCommand::removeConflictingInlineStyleFromRun(EditingStyle* style, RefPtr<Node>& runStart, RefPtr<Node>& runEnd, PassRefPtr<Node> pastEndNode)
+{
+    ASSERT(runStart && runEnd);
     RefPtr<Node> next = runStart;
     for (RefPtr<Node> node = next; node && node->inDocument() && node != pastEndNode; node = next) {
         if (editingIgnoresContent(node.get())) {
@@ -818,8 +864,6 @@ bool ApplyStyleCommand::removeStyleFromRunBeforeApplyingStyle(EditingStyle* styl
                 runEnd = nextSibling ? nextSibling->previousSibling() : parent->lastChild();
         }
     }
-
-    return true;
 }
 
 bool ApplyStyleCommand::removeInlineStyleFromElement(EditingStyle* style, PassRefPtr<HTMLElement> element, InlineStyleRemovalMode mode, EditingStyle* extractedStyle)
@@ -1338,23 +1382,36 @@ void ApplyStyleCommand::addInlineStyleIfNeeded(EditingStyle* style, PassRefPtr<N
 {
     if (!passedStart || !passedEnd || !passedStart->inDocument() || !passedEnd->inDocument())
         return;
-    RefPtr<Node> startNode = passedStart;
-    RefPtr<Node> endNode = passedEnd;
 
+    RefPtr<Node> start = passedStart;
+    RefPtr<Node> dummyElement;
+    StyleChange styleChange(style, positionToComputeInlineStyleChange(start, dummyElement));
+
+    if (dummyElement)
+        removeNode(dummyElement);
+
+    applyInlineStyleChange(start, passedEnd, styleChange, addStyledElement);
+}
+
+Position ApplyStyleCommand::positionToComputeInlineStyleChange(PassRefPtr<Node> startNode, RefPtr<Node>& dummyElement)
+{
     // It's okay to obtain the style at the startNode because we've removed all relevant styles from the current run.
-    RefPtr<HTMLElement> dummyElement;
     Position positionForStyleComparison;
     if (!startNode->isElementNode()) {
         dummyElement = createStyleSpanElement(document());
         insertNodeAt(dummyElement, positionBeforeNode(startNode.get()));
-        positionForStyleComparison = positionBeforeNode(dummyElement.get());
-    } else
-        positionForStyleComparison = firstPositionInOrBeforeNode(startNode.get());
+        return positionBeforeNode(dummyElement.get());
+    }
 
-    StyleChange styleChange(style, positionForStyleComparison);
+    return firstPositionInOrBeforeNode(startNode.get());
+}
 
-    if (dummyElement)
-        removeNode(dummyElement);
+void ApplyStyleCommand::applyInlineStyleChange(PassRefPtr<Node> passedStart, PassRefPtr<Node> passedEnd, StyleChange& styleChange, EAddStyledElement addStyledElement)
+{
+    RefPtr<Node> startNode = passedStart;
+    RefPtr<Node> endNode = passedEnd;
+    ASSERT(startNode->inDocument());
+    ASSERT(endNode->inDocument());
 
     // Find appropriate font and span elements top-down.
     HTMLElement* fontContainer = 0;
index 58bc19b..6dbe458 100644 (file)
@@ -76,7 +76,8 @@ private:
 
     // style-removal helpers
     bool isStyledInlineElementToRemove(Element*) const;
-    bool removeStyleFromRunBeforeApplyingStyle(EditingStyle*, RefPtr<Node>& runStart, RefPtr<Node>& runEnd);
+    bool shouldApplyInlineStyleToRun(EditingStyle*, Node* runStart, Node* pastEndNode);
+    void removeConflictingInlineStyleFromRun(EditingStyle*, RefPtr<Node>& runStart, RefPtr<Node>& runEnd, PassRefPtr<Node> pastEndNode);
     bool removeInlineStyleFromElement(EditingStyle*, PassRefPtr<HTMLElement>, InlineStyleRemovalMode = RemoveIfNeeded, EditingStyle* extractedStyle = 0);
     inline bool shouldRemoveInlineStyleFromElement(EditingStyle* style, HTMLElement* element) {return removeInlineStyleFromElement(style, element, RemoveNone);}
     void replaceWithSpanOrRemoveIfWithoutAttributes(HTMLElement*&);
@@ -97,6 +98,8 @@ private:
     void applyInlineStyleToNodeRange(EditingStyle*, PassRefPtr<Node> startNode, PassRefPtr<Node> pastEndNode);
     void addBlockStyle(const StyleChange&, HTMLElement*);
     void addInlineStyleIfNeeded(EditingStyle*, PassRefPtr<Node> start, PassRefPtr<Node> end, EAddStyledElement = AddStyledElement);
+    Position positionToComputeInlineStyleChange(PassRefPtr<Node>, RefPtr<Node>& dummyElement);
+    void applyInlineStyleChange(PassRefPtr<Node> startNode, PassRefPtr<Node> endNode, StyleChange&, EAddStyledElement);
     void splitTextAtStart(const Position& start, const Position& end);
     void splitTextAtEnd(const Position& start, const Position& end);
     void splitTextElementAtStart(const Position& start, const Position& end);
index 7f990f5..e9e53a9 100644 (file)
@@ -173,6 +173,15 @@ private:
 
 class StyleChange {
 public:
+    StyleChange()
+        : m_applyBold(false)
+        , m_applyItalic(false)
+        , m_applyUnderline(false)
+        , m_applyLineThrough(false)
+        , m_applySubscript(false)
+        , m_applySuperscript(false)
+    { }
+
     StyleChange(EditingStyle*, const Position&);
 
     String cssStyle() const { return m_cssStyle; }