LayoutTests:
authordarin <darin@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Oct 2006 17:39:43 +0000 (17:39 +0000)
committerdarin <darin@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Oct 2006 17:39:43 +0000 (17:39 +0000)
        Reviewed by Anders.

        - removed non-deterministic behavior in editing that was causing
          give inconsistent results for editing/pasteboard/copy-paste-bidi.html

        * editing/pasteboard/copy-paste-bidi-expected.txt: New results, without the
        anonymous block that sometimes appeared and sometimes did not.

WebCore:

        Reviewed by Anders.

        - removed non-deterministic behavior in editing that was causing
          give inconsistent results for editing/pasteboard/copy-paste-bidi.html

        * editing/ReplaceSelectionCommand.cpp:
        (WebCore::ReplaceSelectionCommand::removeRedundantStyles):
        Use two vectors instead of a map: more efficient, deterministic. There was no
        reason to use a hash table.

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

LayoutTests/ChangeLog
LayoutTests/editing/pasteboard/copy-paste-bidi-expected.txt
WebCore/ChangeLog
WebCore/editing/ReplaceSelectionCommand.cpp

index aacf4b336d504e05c193d0081b26fca183928c5c..2f883888c761adc7f4356cb5cf0161aa785155f6 100644 (file)
@@ -1,3 +1,13 @@
+2006-10-26  Darin Adler  <darin@apple.com>
+
+        Reviewed by Anders.
+
+        - removed non-deterministic behavior in editing that was causing
+          give inconsistent results for editing/pasteboard/copy-paste-bidi.html
+
+        * editing/pasteboard/copy-paste-bidi-expected.txt: New results, without the
+        anonymous block that sometimes appeared and sometimes did not.
+
 2006-10-26  Darin Adler  <darin@apple.com>
 
         - oops, checked in failure results, not success
index b1d7cef49540c08e35d3bfb36af3539de3a33dc7..8fb5e354f67b1c12de37d7b00a6d3df8374b7674 100644 (file)
@@ -49,7 +49,6 @@ layer at (0,0) size 800x600
               text run at (0,0) width 32: "1234"
               text run at (32,0) width 36 RTL: "\x{5E9}\x{5D3}\x{5D2}\x{5DB}"
               text run at (68,0) width 4: ":"
-          RenderBlock (anonymous) at (0,18) size 784x0
           RenderBlock {DIV} at (0,18) size 784x18
             RenderText {#text} at (712,0) size 72x18
               text run at (712,0) width 40 RTL: "\x{5E9}\x{5D3}\x{5D2}\x{5DB}:"
index edc38376f3f873148619eda783215c05454faee8..7569015332a5cb422c59b9c13c8573ec309d5d77 100644 (file)
@@ -1,3 +1,15 @@
+2006-10-26  Darin Adler  <darin@apple.com>
+
+        Reviewed by Anders.
+
+        - removed non-deterministic behavior in editing that was causing
+          give inconsistent results for editing/pasteboard/copy-paste-bidi.html
+
+        * editing/ReplaceSelectionCommand.cpp:
+        (WebCore::ReplaceSelectionCommand::removeRedundantStyles):
+        Use two vectors instead of a map: more efficient, deterministic. There was no
+        reason to use a hash table.
+
 2006-10-26  Maciej Stachowiak  <mjs@apple.com>
 
         Reviewed by Anders.
index e1b6226f83ea0cc39886a95a43a32d1541deaf97..607da55b86edd62f492f6b9c61bded5bea1f173d 100644 (file)
@@ -315,18 +315,15 @@ bool ReplaceSelectionCommand::shouldMerge(const VisiblePosition& from, const Vis
 
 void ReplaceSelectionCommand::removeRedundantStyles()
 {
-    typedef HashMap<Node*, RefPtr<CSSMutableStyleDeclaration> > NodeStyleMap;
-
-    Node* node = m_firstNodeInserted.get();
-    
     // There's usually a top level style span that holds the document's default style, push it down.
+    Node* node = m_firstNodeInserted.get();
     if (isStyleSpan(node)) {
-        
-        RefPtr<CSSMutableStyleDeclaration> parentStyle = Position(node, 0).computedStyle()->copyInheritableProperties();
-        
+        RefPtr<CSSMutableStyleDeclaration> parentStyle
+            = Position(node, 0).computedStyle()->copyInheritableProperties();
+
         RefPtr<Node> child = node->firstChild();
         while (child) {
-            Node* next = child->nextSibling();
+            RefPtr<Node> next = child->nextSibling();
             if (isStyleSpan(child.get())) {
                 HTMLElement* elem = static_cast<HTMLElement*>(child.get());
                 CSSMutableStyleDeclaration* inlineStyleDecl = elem->inlineStyleDecl();
@@ -347,7 +344,7 @@ void ReplaceSelectionCommand::removeRedundantStyles()
             }
             child = next;
         }
-        
+
         if (m_firstNodeInserted == node)
             m_firstNodeInserted = node->firstChild();
         if (m_lastNodeInserted == node)
@@ -355,71 +352,63 @@ void ReplaceSelectionCommand::removeRedundantStyles()
         removeNodePreservingChildren(node);
     }
     
-    node = m_firstNodeInserted.get();
-    
-    // Compute and save the non-redundant styles for all Elements.  Don't do any mutation here, because
-    // that would cause the diffs to trigger layouts.
-    NodeStyleMap map;
-    while (node) {
-        Node* next = node->traverseNextNode();
-        
-        if (!node->isHTMLElement()) {
-            node = next;
-            continue;
+    // Compute and save the non-redundant styles for all HTML elements.
+    // Don't do any mutation here, because that would cause the diffs to trigger layouts.
+    Vector<RefPtr<CSSMutableStyleDeclaration> > styles;
+    Vector<RefPtr<HTMLElement> > elements;
+    for (node = m_firstNodeInserted.get(); node; node = node->traverseNextNode()) {
+        if (node->isHTMLElement()) {
+            elements.append(static_cast<HTMLElement*>(node));
+            RefPtr<CSSMutableStyleDeclaration> style
+                = Position(node, 0).computedStyle()->copyInheritableProperties();
+            RefPtr<CSSMutableStyleDeclaration> parentStyle
+                = Position(node->parentNode(), 0).computedStyle()->copyInheritableProperties();
+            parentStyle->diff(style.get());
+            styles.append(style.release());
         }
-        
-        RefPtr<CSSMutableStyleDeclaration> style = Position(node, 0).computedStyle()->copyInheritableProperties();
-        RefPtr<CSSMutableStyleDeclaration> parentStyle = Position(node->parentNode(), 0).computedStyle()->copyInheritableProperties();
-        
-        parentStyle->diff(style.get());
-        map.add(node, style);
-        
-        if (node == m_lastNodeInserted.get())
+        if (node == m_lastNodeInserted)
             break;
-            
-        node = next;
     }
     
-    NodeStyleMap::const_iterator e = map.end();
-    for (NodeStyleMap::const_iterator it = map.begin(); it != e; ++it) {
-        Node* node = it->first;
-        
-        // This node could have aleady been removed during pruning if one 
-        // of its descendants came off of the hash map iterator before it 
-        // and that descendant was a redundant style span or font tag.
-        if (!node->inDocument())
+    size_t count = styles.size();
+    for (size_t i = 0; i < count; ++i) {
+        HTMLElement* element = elements[i].get();
+
+        // Handle case where the element was already removed by earlier processing.
+        // It's possible this no longer occurs, but it did happen in an earlier version
+        // that processed elements in a less-determistic order, and I can't prove it
+        // does not occur.
+        if (!element->inDocument())
             continue;
-        
+
         // Remove empty style spans.
-        if (isStyleSpan(node) && !node->hasChildNodes()) {
-            removeNodeAndPruneAncestors(node);
+        if (isStyleSpan(element) && !element->hasChildNodes()) {
+            removeNodeAndPruneAncestors(element);
             continue;
         }
-        
-        RefPtr<CSSMutableStyleDeclaration> style = it->second;
-        if (style->length() == 0) {
-            // Remove redundant style tags and style spans.
-            if (isStyleSpan(node) ||
-                node->hasTagName(bTag) ||
-                node->hasTagName(fontTag) ||
-                node->hasTagName(iTag) ||
-                node->hasTagName(uTag)) {
-                if (node == m_firstNodeInserted.get())
-                    m_firstNodeInserted = node->traverseNextNode();
-                if (node == m_lastNodeInserted.get())
-                    m_lastNodeInserted = node->traverseNextNode();
-                removeNodePreservingChildren(node);
-                continue;
-            }
+
+        // Remove redundant style tags and style spans.
+        CSSMutableStyleDeclaration* style = styles[i].get();
+        if (style->length() == 0
+                && (isStyleSpan(element)
+                    || element->hasTagName(bTag)
+                    || element->hasTagName(fontTag)
+                    || element->hasTagName(iTag)
+                    || element->hasTagName(uTag))) {
+            if (element == m_firstNodeInserted)
+                m_firstNodeInserted = element->traverseNextNode();
+            if (element == m_lastNodeInserted)
+                m_lastNodeInserted = element->traverseNextNode();
+            removeNodePreservingChildren(element);
+            continue;
         }
-        
+
         // Clear redundant styles from elements.
-        HTMLElement* elem = static_cast<HTMLElement*>(node);
-        CSSMutableStyleDeclaration* inlineStyleDecl = elem->inlineStyleDecl();
+        CSSMutableStyleDeclaration* inlineStyleDecl = element->inlineStyleDecl();
         if (inlineStyleDecl) {
             inlineStyleDecl->removeInheritableProperties();
-            inlineStyleDecl->merge(style.get(), true);
-            setNodeAttribute(elem, styleAttr, inlineStyleDecl->cssText());
+            inlineStyleDecl->merge(style, true);
+            setNodeAttribute(element, styleAttr, inlineStyleDecl->cssText());
         }
     }
 }