2011-05-20 MORITA Hajime <morrita@google.com>

Reviewed by Dimitri Glazkov. [Refactoring] attach() following detach() should be replaced with Node::reattach() https://bugs.webkit.org/show_bug.cgi?id=61011 - Renamed forceReattach() to reattach() - Introduced reattachIfAttached() as a variant. No new tests. No behavior change. * dom/CharacterData.cpp: (WebCore::CharacterData::updateRenderer): * dom/Element.cpp: (WebCore::Element::recalcStyle): * dom/Node.h: (WebCore::Node::reattach): (WebCore::Node::reattachIfAttached): * dom/Text.cpp: (WebCore::Text::recalcStyle): * html/HTMLDetailsElement.cpp: (WebCore::HTMLDetailsElement::refreshMainSummary): (WebCore::HTMLDetailsElement::parseMappedAttribute): * html/HTMLInputElement.cpp: (WebCore::HTMLInputElement::parseMappedAttribute): * html/HTMLObjectElement.cpp: (WebCore::HTMLObjectElement::renderFallbackContent): * html/HTMLPlugInImageElement.cpp: (WebCore::HTMLPlugInImageElement::recalcStyle): * html/HTMLSelectElement.cpp: (WebCore::HTMLSelectElement::parseMappedAttribute): Canonical link: https://commits.webkit.org/76704@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@87123 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · May 24, 2011 · d7e63987e935dba9bdd25bd8bdfb6b94ae7136f1 · d7e6398
1 parent c937ee6
commit d7e63987e935dba9bdd25bd8bdfb6b94ae7136f1
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,36 @@
+2011-05-20  MORITA Hajime  <morrita@google.com>
+
+        Reviewed by Dimitri Glazkov.
+
+        [Refactoring] attach() following detach() should be replaced with Node::reattach()
+        https://bugs.webkit.org/show_bug.cgi?id=61011
+
+        - Renamed forceReattach() to reattach()
+        - Introduced reattachIfAttached() as a variant.
+
+        No new tests. No behavior change.
+
+        * dom/CharacterData.cpp:
+        (WebCore::CharacterData::updateRenderer):
+        * dom/Element.cpp:
+        (WebCore::Element::recalcStyle):
+        * dom/Node.h:
+        (WebCore::Node::reattach):
+        (WebCore::Node::reattachIfAttached):
+        * dom/Text.cpp:
+        (WebCore::Text::recalcStyle):
+        * html/HTMLDetailsElement.cpp:
+        (WebCore::HTMLDetailsElement::refreshMainSummary):
+        (WebCore::HTMLDetailsElement::parseMappedAttribute):
+        * html/HTMLInputElement.cpp:
+        (WebCore::HTMLInputElement::parseMappedAttribute):
+        * html/HTMLObjectElement.cpp:
+        (WebCore::HTMLObjectElement::renderFallbackContent):
+        * html/HTMLPlugInImageElement.cpp:
+        (WebCore::HTMLPlugInImageElement::recalcStyle):
+        * html/HTMLSelectElement.cpp:
+        (WebCore::HTMLSelectElement::parseMappedAttribute):
+
 2011-05-23  Mark Rowe  <mrowe@apple.com>
 
         Build fix after r87117.

diff --git a/Source/WebCore/dom/CharacterData.cpp b/Source/WebCore/dom/CharacterData.cpp
@@ -182,8 +182,7 @@ void CharacterData::setDataAndUpdate(PassRefPtr<StringImpl> newData, unsigned of
 void CharacterData::updateRenderer(unsigned offsetOfReplacedData, unsigned lengthOfReplacedData)
 {
     if ((!renderer() || !rendererIsNeeded(renderer()->style())) && attached()) {
-        detach();
-        attach();
+        reattach();
     } else if (renderer())
         toRenderText(renderer())->setTextWithOffset(m_data, offsetOfReplacedData, lengthOfReplacedData);
 }

diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
@@ -1086,9 +1086,8 @@ void Element::recalcStyle(StyleChange change)
         RefPtr<RenderStyle> newStyle = styleForRenderer();
         StyleChange ch = diff(currentStyle.get(), newStyle.get());
         if (ch == Detach || !currentStyle) {
-            if (attached())
-                detach();
-            attach(); // FIXME: The style gets computed twice by calling attach. We could do better if we passed the style along.
+            // FIXME: The style gets computed twice by calling attach. We could do better if we passed the style along.
+            reattach();
             // attach recalulates the style for all children. No need to do it twice.
             clearNeedsStyleRecalc();
             clearChildNeedsStyleRecalc();

diff --git a/Source/WebCore/dom/Node.h b/Source/WebCore/dom/Node.h
@@ -451,7 +451,8 @@ class Node : public EventTarget, public TreeShared<ContainerNode>, public Script
     // the node's rendering object from the rendering tree and delete it.
     virtual void detach();
 
-    void forceReattach();
+    void reattach();
+    void reattachIfAttached();
 
     virtual void willRemove();
     void createRendererIfNeeded();
@@ -739,14 +740,19 @@ inline ContainerNode* Node::parentNodeGuaranteedHostFree() const
     return parentOrHostNode();
 }
 
-inline void Node::forceReattach()
+inline void Node::reattach()
 {
-    if (!attached())
-        return;
-    detach();
+    if (attached())
+        detach();
     attach();
 }
 
+inline void Node::reattachIfAttached()
+{
+    if (attached())
+        reattach();
+}
+
 } //namespace
 
 #ifndef NDEBUG

diff --git a/Source/WebCore/dom/ShadowContentSelector.cpp b/Source/WebCore/dom/ShadowContentSelector.cpp
@@ -62,7 +62,7 @@ void ShadowContentSelector::attachChildrenFor(ShadowContentElement* contentEleme
         if (!contentElement->shouldInclude(child))
             continue;
 
-        child->forceReattach();
+        child->reattach();
         m_children[i] = 0;
     }
 

diff --git a/Source/WebCore/dom/ShadowRoot.cpp b/Source/WebCore/dom/ShadowRoot.cpp
@@ -84,7 +84,7 @@ bool ShadowRoot::childTypeAllowed(NodeType type) const
 void ShadowRoot::recalcStyle(StyleChange change)
 {
     if (hasContentElement())
-        forceReattach();
+        reattach();
     else {
         for (Node* n = firstChild(); n; n = n->nextSibling())
             n->recalcStyle(change);

diff --git a/Source/WebCore/dom/Text.cpp b/Source/WebCore/dom/Text.cpp
@@ -265,11 +265,8 @@ void Text::recalcStyle(StyleChange change)
         if (renderer()) {
             if (renderer()->isText())
                 toRenderText(renderer())->setText(dataImpl());
-        } else {
-            if (attached())
-                detach();
-            attach();
-        }
+        } else
+            reattach();
     }
     clearNeedsStyleRecalc();
 }

diff --git a/Source/WebCore/html/HTMLDetailsElement.cpp b/Source/WebCore/html/HTMLDetailsElement.cpp
@@ -146,15 +146,10 @@ void HTMLDetailsElement::refreshMainSummary(RefreshRenderer refreshRenderer)
     if (oldSummary == m_mainSummary || !attached())
         return;
 
-    if (oldSummary && oldSummary->parentNodeForRenderingAndStyle()) {
-        oldSummary->detach();
-        oldSummary->attach();
-    }
-
-    if (m_mainSummary && refreshRenderer == RefreshRendererAllowed) {
-        m_mainSummary->detach();
-        m_mainSummary->attach();
-    }
+    if (oldSummary && oldSummary->parentNodeForRenderingAndStyle())
+        oldSummary->reattach();
+    if (m_mainSummary && refreshRenderer == RefreshRendererAllowed)
+        m_mainSummary->reattach();
 }
 
 void HTMLDetailsElement::createShadowSubtree()
@@ -196,10 +191,8 @@ void HTMLDetailsElement::parseMappedAttribute(Attribute* attr)
     if (attr->name() == openAttr) {
         bool oldValue = m_isOpen;
         m_isOpen =  !attr->value().isNull();
-        if (attached() && oldValue != m_isOpen) {
-            detach();
-            attach();
-        }
+        if (oldValue != m_isOpen)
+            reattachIfAttached();
     } else
         HTMLElement::parseMappedAttribute(attr);
 }

diff --git a/Source/WebCore/html/HTMLInputElement.cpp b/Source/WebCore/html/HTMLInputElement.cpp
@@ -721,10 +721,8 @@ void HTMLInputElement::parseMappedAttribute(Attribute* attr)
         m_maxResults = !attr->isNull() ? std::min(attr->value().toInt(), maxSavedResults) : -1;
         // FIXME: Detaching just for maxResults change is not ideal.  We should figure out the right
         // time to relayout for this change.
-        if (m_maxResults != oldResults && (m_maxResults <= 0 || oldResults <= 0) && attached()) {
-            detach();
-            attach();
-        }
+        if (m_maxResults != oldResults && (m_maxResults <= 0 || oldResults <= 0))
+            reattachIfAttached();
         setNeedsStyleRecalc();
     } else if (attr->name() == autosaveAttr || attr->name() == incrementalAttr)
         setNeedsStyleRecalc();

diff --git a/Source/WebCore/html/HTMLObjectElement.cpp b/Source/WebCore/html/HTMLObjectElement.cpp
@@ -378,9 +378,8 @@ void HTMLObjectElement::renderFallbackContent()
         m_serviceType = m_imageLoader->image()->response().mimeType();
         if (!isImageType()) {
             // If we don't think we have an image type anymore, then clear the image from the loader.
-            m_imageLoader->setImage(0);        
-            detach();
-            attach();
+            m_imageLoader->setImage(0);
+            reattach();
             return;
         }
     }

diff --git a/Source/WebCore/html/HTMLPlugInImageElement.cpp b/Source/WebCore/html/HTMLPlugInImageElement.cpp
@@ -124,10 +124,8 @@ RenderObject* HTMLPlugInImageElement::createRenderer(RenderArena* arena, RenderS
 void HTMLPlugInImageElement::recalcStyle(StyleChange ch)
 {
     // FIXME: Why is this necessary?  Manual re-attach is almost always wrong.
-    if (!useFallbackContent() && needsWidgetUpdate() && renderer() && !isImageType()) {
-        detach();
-        attach();
-    }
+    if (!useFallbackContent() && needsWidgetUpdate() && renderer() && !isImageType())
+        reattach();
     HTMLPlugInElement::recalcStyle(ch);
 }
 

diff --git a/Source/WebCore/html/HTMLSelectElement.cpp b/Source/WebCore/html/HTMLSelectElement.cpp
@@ -264,8 +264,7 @@ void HTMLSelectElement::parseMappedAttribute(Attribute* attr)
         m_data.setSize(size);
         setNeedsValidityCheck();
         if ((oldUsesMenuList != m_data.usesMenuList() || (!oldUsesMenuList && m_data.size() != oldSize)) && attached()) {
-            detach();
-            attach();
+            reattach();
             setRecalcListItems();
         }
     } else if (attr->name() == multipleAttr)