Don't expand <symbol> elements in <use> that are not targets

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 287 matching lines @@
         return true;
 
     for (const Node* cur = start->firstChild(); cur; cur = cur->nextSibling()) {
         if (subtreeContainsDisallowedElement(cur))
             return true;
     }
 
     return false;
 }
 
+static inline void removeSymbolElementsFromSubtree(SVGElement& subtreeRoot)
+{
+    Element* element = ElementTraversal::firstWithin(subtreeRoot);
+    while (element) {
+        if (isSVGSymbolElement(element)) {
+            Element* next = ElementTraversal::nextSkippingChildren(*element, &subtreeRoot);
+            // The subtree is not in document so this won't generate events that could mutate the tree.

[pdr., 2016/02/29 23:45:40]

Can you assert this here and/or above--that the subtree is not in the document?

+            element->parentNode()->removeChild(element);
+            element = next;
+        } else {
+            element = ElementTraversal::next(*element, &subtreeRoot);
+        }
+    }
+}
+
 void SVGUseElement::scheduleShadowTreeRecreation()
 {
     if (inUseShadowTree())
         return;
     m_needsShadowTreeRecreation = true;
     document().scheduleUseShadowTreeUpdate(*this);
 }
 
 void SVGUseElement::cancelShadowTreeRecreation()
 {
@@ skipping 78 matching lines @@
     if (inUseShadowTree())
         return;
 
     // Do not allow self-referencing.
     // 'target' may be null, if it's a non SVG namespaced element.
     if (!target || target == this || isDisallowedElement(target))
         return;
 
     // Set up root SVG element in shadow tree.
     RefPtrWillBeRawPtr<Element> newChild = target->cloneElementWithoutChildren();
-    m_targetElementInstance = toSVGElement(newChild.get());
-    ShadowRoot* shadowTreeRootElement = userAgentShadowRoot();
-    shadowTreeRootElement->appendChild(newChild.release());
+    ASSERT(newChild->isSVGElement());
 
     // Clone the target subtree into the shadow tree, not handling <use> and <symbol> yet.
 
     // SVG specification does not say a word about <use> & cycles. My view on this is: just ignore it!
     // Non-appearing <use> content is easier to debug, then half-appearing content.
-    if (!buildShadowTree(target, m_targetElementInstance.get(), false)) {
+    if (!buildShadowTree(target, toSVGElement(newChild.get()), false)) {
         clearShadowTree();
         return;
     }
+    // Remove any <symbol> elements from the clone, because if not, they'll be
+    // replaced by <svg> elements which will appear in the rendered result (and
+    // <symbol>s that are not a direct target of a <use> should not be
+    // rendered.)
+    removeSymbolElementsFromSubtree(toSVGElement(*newChild));

[pdr., 2016/02/29 23:45:40]

expandSymbolElementsInShadowTree is called below and does another full subtree
walk--is that necessary now or can we just check expand the target if it is a
symbol?

+
+    // Attach the new clone to our shadow root.
+    m_targetElementInstance = toSVGElement(newChild.get());
+    ShadowRoot* shadowTreeRootElement = userAgentShadowRoot();
+    shadowTreeRootElement->appendChild(newChild.release());
 
     if (instanceTreeIsLoading(m_targetElementInstance.get()))
         return;
 
     // Assure shadow tree building was successfull
     ASSERT(m_targetElementInstance);
     ASSERT(m_targetElementInstance->correspondingUseElement() == this);
     ASSERT(m_targetElementInstance->correspondingElement() == target);
 
     // Expand all <use> elements in the shadow tree.
@@ skipping 187 matching lines @@
             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, cloneParent.get());
 
         if (target) {
             RefPtrWillBeRawPtr<Node> newChild = cloneNodeAndAssociate(*target);
             ASSERT(newChild->isSVGElement());
+            // Remove any <symbol> elements from the clone, because if not,

[pdr., 2016/02/29 23:45:40]

I like how webkit wraps this complexity into a single location (cloneTarget)
instead of having to duplicate it twice. Do you think we could make a similar
change by extracting out this common functionality
(removeSymbolElementsFromSubtree, transferUseWidthAndHeightIfNeeded,
removeDisallowedElementsFromSubtree, etc)?

+            // they'll be replaced by <svg> elements which will appear in the
+            // rendered result (and <symbol>s that are not a direct target of a
+            // <use> should not be rendered.)
+            removeSymbolElementsFromSubtree(toSVGElement(*newChild));
             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
         // 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(cloneParent.get()))
@@ skipping 200 matching lines @@
 
     if (m_resource)
         m_resource->removeClient(this);
 
     m_resource = resource;
     if (m_resource)
         m_resource->addClient(this);
 }
 
 } // namespace blink