Clean up reparenting in SVGUseElement::expand*

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

 /*
  * Copyright (C) 2004, 2005, 2006, 2007, 2008 Nikolas Zimmermann <zimmermann@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007 Rob Buis <buis@kde.org>
  * Copyright (C) Research In Motion Limited 2009-2010. All rights reserved.
  * Copyright (C) 2011 Torch Mobile (Beijing) Co. Ltd. All rights reserved.
  * Copyright (C) 2012 University of Szeged
  * Copyright (C) 2012 Renata Hodovan <reni@webkit.org>
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 568 matching lines @@
             Element* next = ElementTraversal::nextSkippingChildren(*element, &subtree);
             // The subtree is not in document so this won't generate events that could mutate the tree.
             element->parentNode()->removeChild(element);
             element = next;
         } else {
             element = ElementTraversal::next(*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
     // do this directly in buildShadowTree, if we encounter a <use> element?
     //
     // Short answer: Because we may miss to expand some elements. For example, if a <symbol>
     // contains <use> tags, we'd miss them. So once we're done with setting up the
     // actual shadow tree (after the special case modification for svg/symbol) we have
     // to walk it completely and expand all <use> elements.
     ShadowRoot* shadowRoot = userAgentShadowRoot();
     for (RefPtrWillBeRawPtr<SVGUseElement> use = Traversal<SVGUseElement>::firstWithin(*shadowRoot); use; ) {
         ASSERT(!use->resourceIsStillLoading());
 
         SVGElement* target = 0;
         if (hasCycleUseReferencing(toSVGUseElement(use->correspondingElement()), use.get(), target))
             return false;
 
         if (target && isDisallowedElement(target))
             return false;
         // 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);
             ASSERT(newChild->isSVGElement());
             transferUseWidthAndHeightIfNeeded(*use, toSVGElement(newChild.get()), *target);
             cloneParent->appendChild(newChild.release());
         }
 
         // We don't walk the target tree element-by-element, and clone each element,
         // but instead use cloneElementWithChildren(). This is an optimization for the common
@@ skipping 22 matching lines @@
         // always have explicit values for attributes width and height. If attributes width and/or
         // height are provided on the 'use' element, then these attributes will be transferred to
         // the generated 'svg'. If attributes width and/or height are not specified, the generated
         // 'svg' element will use values of 100% for these attributes.
         ASSERT(referencedScope());
         RefPtrWillBeRawPtr<SVGSVGElement> svgElement = SVGSVGElement::create(referencedScope()->document());
         // Transfer all data (attributes, etc.) from <symbol> to the new <svg> element.
         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
         // case where <use> doesn't contain disallowed elements (ie. <foreignObject>).
         // Though if there are disallowed elements in the subtree, we have to remove them.
         // For instance: <use> on <g> containing <foreignObject> (indirect case).
         if (subtreeContainsDisallowedElement(svgElement.get()))
             removeDisallowedElementsFromSubtree(*svgElement);
 
         RefPtrWillBeRawPtr<SVGElement> replacingElement(svgElement.get());
@@ skipping 21 matching lines @@
     WillBeHeapVector<RefPtrWillBeMember<SVGElement>> instances;
     instances.appendRange(rawInstances.begin(), rawInstances.end());
     for (auto& instance : instances) {
         if (RefPtrWillBeRawPtr<SVGUseElement> element = instance->correspondingUseElement()) {
             ASSERT(element->inDocument());
             element->invalidateShadowTree();
         }
     }
 }
 
-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()
         || m_y->currentValue()->isRelative()
         || m_width->currentValue()->isRelative()
         || m_height->currentValue()->isRelative())
         return true;
 
     if (!m_targetElementInstance)
         return false;
@@ skipping 80 matching lines @@
 
     if (m_resource)
         m_resource->removeClient(this);
 
     m_resource = resource;
     if (m_resource)
         m_resource->addClient(this);
 }
 
 } // namespace blink