Clean up reparenting in SVGUseElement::expand*

third_party/WebKit/Source/core/svg/SVGUseElement.cpp

Index: third_party/WebKit/Source/core/svg/SVGUseElement.cpp
diff --git a/third_party/WebKit/Source/core/svg/SVGUseElement.cpp b/third_party/WebKit/Source/core/svg/SVGUseElement.cpp
index 248c716cee1b86d7523e613af94a5e819d027b5a..eb77b75fc50c6e77b745013310a801e8bd3df4fe 100644
--- a/third_party/WebKit/Source/core/svg/SVGUseElement.cpp
+++ b/third_party/WebKit/Source/core/svg/SVGUseElement.cpp
@@ -586,6 +586,25 @@ static inline void removeDisallowedElementsFromSubtree(Element& subtree)
     }
 }
 
+static void moveChildrenToReplacementElement(ContainerNode& sourceRoot, ContainerNode& destinationRoot)
+{
+    for (RefPtrWillBeRawPtr<Node> child = sourceRoot.firstChild(); child; ) {
+        RefPtrWillBeRawPtr<Node> nextChild = child->nextSibling();
+        destinationRoot.appendChild(child);
+        child = nextChild.release();
+    }
+}
+
+static void removeAttributesFromReplacementElement(SVGElement& to)

[pdr., 2016/03/02 21:24:35]

It looks like this area is primed to extract out the common code between the two
expand functions. If that's the case, please don't waste your time on my nits
since they will be resolved in a followup.

Nits: Can you move the comments about what is being removed up here? The name
'to' is a little confusing too.

[fs, 2016/03/02 21:30:01]

I suspect this function might stay alive even after followups, so I better fix
these nits up.

+{
+    to.removeAttribute(SVGNames::xAttr);
+    to.removeAttribute(SVGNames::yAttr);
+    to.removeAttribute(SVGNames::widthAttr);
+    to.removeAttribute(SVGNames::heightAttr);
+    to.removeAttribute(SVGNames::hrefAttr);
+    to.removeAttribute(XLinkNames::hrefAttr);
+}
+
 bool SVGUseElement::expandUseElementsInShadowTree()
 {
     // Why expand the <use> elements in the shadow tree here, and not just
@@ -608,18 +627,16 @@ bool SVGUseElement::expandUseElementsInShadowTree()
         // Don't ASSERT(target) here, it may be "pending", too.
         // Setup sub-shadow tree root node
         RefPtrWillBeRawPtr<SVGGElement> cloneParent = SVGGElement::create(referencedScope()->document());
+        // Transfer all data (attributes, etc.) from <use> to the new <g> element.
+        cloneParent->cloneDataFromElement(*use);
         cloneParent->setCorrespondingElement(use->correspondingElement());
 
-        // Move already cloned elements to the new <g> element
-        for (RefPtrWillBeRawPtr<Node> child = use->firstChild(); child; ) {
-            RefPtrWillBeRawPtr<Node> nextChild = child->nextSibling();
-            cloneParent->appendChild(child);
-            child = nextChild.release();
-        }
-
         // Spec: In the generated content, the 'use' will be replaced by 'g', where all attributes from the
         // 'use' element except for x, y, width, height and xlink:href are transferred to the generated 'g' element.
-        transferUseAttributesToReplacedElement(use.get(), cloneParent.get());
+        removeAttributesFromReplacementElement(*cloneParent);
+
+        // Move already cloned elements to the new <g> element.
+        moveChildrenToReplacementElement(*use, *cloneParent);
 
         if (target) {
             RefPtrWillBeRawPtr<Node> newChild = cloneNodeAndAssociate(*target);
@@ -662,12 +679,8 @@ void SVGUseElement::expandSymbolElementsInShadowTree()
         svgElement->cloneDataFromElement(*symbol);
         svgElement->setCorrespondingElement(symbol->correspondingElement());
 
-        // Move already cloned elements to the new <svg> element
-        for (RefPtrWillBeRawPtr<Node> child = symbol->firstChild(); child; ) {
-            RefPtrWillBeRawPtr<Node> nextChild = child->nextSibling();
-            svgElement->appendChild(child);
-            child = nextChild.release();
-        }
+        // Move already cloned elements to the new <svg> element.
+        moveChildrenToReplacementElement(*symbol, *svgElement);
 
         // We don't walk the target tree element-by-element, and clone each element,
         // but instead use cloneNode(deep=true). This is an optimization for the common
@@ -709,21 +722,6 @@ void SVGUseElement::invalidateDependentShadowTrees()
     }
 }
 
-void SVGUseElement::transferUseAttributesToReplacedElement(SVGElement* from, SVGElement* to) const
-{
-    ASSERT(from);
-    ASSERT(to);
-
-    to->cloneDataFromElement(*from);
-
-    to->removeAttribute(SVGNames::xAttr);
-    to->removeAttribute(SVGNames::yAttr);
-    to->removeAttribute(SVGNames::widthAttr);
-    to->removeAttribute(SVGNames::heightAttr);
-    to->removeAttribute(SVGNames::hrefAttr);
-    to->removeAttribute(XLinkNames::hrefAttr);
-}
-
 bool SVGUseElement::selfHasRelativeLengths() const
 {
     if (m_x->currentValue()->isRelative()