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

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 848977a8c55afef5d40533597c9cc9bee470dcb2..3677fc8a0d38bc2966714ed627c48bb02e38558c 100644
--- a/third_party/WebKit/Source/core/svg/SVGUseElement.cpp
+++ b/third_party/WebKit/Source/core/svg/SVGUseElement.cpp
@@ -305,6 +305,21 @@ static bool subtreeContainsDisallowedElement(const Node* start)
     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())
@@ -403,18 +418,26 @@ void SVGUseElement::buildShadowAndInstanceTree(SVGElement* target)
 
     // 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;
@@ -622,6 +645,11 @@ bool SVGUseElement::expandUseElementsInShadowTree(SVGElement* element)
         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());
         }