Do refactor in collectGradientAttributes() and renderStyleForLengthResolve()

Source/core/svg/SVGRadialGradientElement.cpp

 /*
  * Copyright (C) 2004, 2005, 2006, 2008 Nikolas Zimmermann <zimmermann@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007 Rob Buis <buis@kde.org>
  * Copyright (C) 2008 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2008 Dirk Schulze <krit@webkit.org>
  * Copyright (C) Research In Motion Limited 2010. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
@@ skipping 116 matching lines @@
 
 RenderObject* SVGRadialGradientElement::createRenderer(RenderStyle*)
 {
     return new RenderSVGResourceRadialGradient(this);
 }
 
 bool SVGRadialGradientElement::collectGradientAttributes(RadialGradientAttributes& attributes)
 {
     HashSet<SVGGradientElement*> processedGradients;
 
-    bool isRadial = true;
     SVGGradientElement* current = this;
+    setAttributeAnimatedLength(current, attributes);
 
-    while (current) {
+    do {
         if (!current->renderer())

[Stephen Chennney, 2014/01/16 02:19:52]

This "if" needs to be done for the initial "this" too.

[gyuyoung-inactive, 2014/01/16 02:34:01]

Right.

             return false;
 
         if (!attributes.hasSpreadMethod() && current->hasAttribute(SVGNames::spreadMethodAttr))

[Stephen Chennney, 2014/01/16 02:19:52]

All of these attribute checks should also go into your new method. It's fine to
call them on a SVGGradientElement.

[gyuyoung-inactive, 2014/01/16 02:34:01]

Yes, it is fine to call them in new method. However, these calls need be called
both *radialGradientElement* and *linearGradientElement*. If we move those to
new method, those are only called when element is *radialGradientElement* in my
patch. It changes existing behavior. I only moved "if (isRadial) { ... }" into
new method. That's why I didn't move this into new method.

             attributes.setSpreadMethod(current->spreadMethodCurrentValue());
 
         if (!attributes.hasGradientUnits() && current->hasAttribute(SVGNames::gradientUnitsAttr))
             attributes.setGradientUnits(current->gradientUnitsCurrentValue());
 
         if (!attributes.hasGradientTransform() && current->hasAttribute(SVGNames::gradientTransformAttr)) {
             AffineTransform transform;
             current->gradientTransformCurrentValue().concatenate(transform);
             attributes.setGradientTransform(transform);
         }
 
         if (!attributes.hasStops()) {
             const Vector<Gradient::ColorStop>& stops(current->buildStops());
             if (!stops.isEmpty())
                 attributes.setStops(stops);
         }
 
-        if (isRadial) {
-            SVGRadialGradientElement* radial = toSVGRadialGradientElement(current);
-
-            if (!attributes.hasCx() && current->hasAttribute(SVGNames::cxAttr))
-                attributes.setCx(radial->cx()->currentValue());
-
-            if (!attributes.hasCy() && current->hasAttribute(SVGNames::cyAttr))
-                attributes.setCy(radial->cy()->currentValue());
-
-            if (!attributes.hasR() && current->hasAttribute(SVGNames::rAttr))
-                attributes.setR(radial->r()->currentValue());
-
-            if (!attributes.hasFx() && current->hasAttribute(SVGNames::fxAttr))
-                attributes.setFx(radial->fx()->currentValue());
-
-            if (!attributes.hasFy() && current->hasAttribute(SVGNames::fyAttr))
-                attributes.setFy(radial->fy()->currentValue());
-
-            if (!attributes.hasFr() && current->hasAttribute(SVGNames::frAttr))
-                attributes.setFr(radial->fr()->currentValue());
-        }
-
         processedGradients.add(current);

[Stephen Chennney, 2014/01/16 02:19:52]

You'll need to do this for the initial "this" element too.

[gyuyoung-inactive, 2014/01/16 02:34:01]

Is *this* element added by first iteration ? I don't understand well why I need
to do this before entering do-while loop.

 
         // Respect xlink:href, take attributes from referenced element
         Node* refNode = SVGURIReference::targetElementFromIRIString(current->hrefCurrentValue(), document());
-        if (refNode && (refNode->hasTagName(SVGNames::radialGradientTag) || refNode->hasTagName(SVGNames::linearGradientTag))) {
+        if (refNode && isSVGGradientElement(*refNode)) {
             current = toSVGGradientElement(refNode);
 
             // Cycle detection
             if (processedGradients.contains(current)) {
                 current = 0;
                 break;
             }
 
-            isRadial = current->hasTagName(SVGNames::radialGradientTag);
-        } else
+            if (current->hasTagName(SVGNames::radialGradientTag))
+                setAttributeAnimatedLength(current, attributes);
+        } else {
             current = 0;
-    }
+        }
+    } while (current);
 
     // Handle default values for fx/fy
     if (!attributes.hasFx())
         attributes.setFx(attributes.cx());
 
     if (!attributes.hasFy())
         attributes.setFy(attributes.cy());
     return true;
 }
 
+void SVGRadialGradientElement::setAttributeAnimatedLength(SVGGradientElement* element, RadialGradientAttributes& attributes)
+{
+    SVGRadialGradientElement* radial = toSVGRadialGradientElement(element);
+
+    if (!attributes.hasCx() && element->hasAttribute(SVGNames::cxAttr))
+        attributes.setCx(radial->cx()->currentValue());
+
+    if (!attributes.hasCy() && element->hasAttribute(SVGNames::cyAttr))
+        attributes.setCy(radial->cy()->currentValue());
+
+    if (!attributes.hasR() && element->hasAttribute(SVGNames::rAttr))
+        attributes.setR(radial->r()->currentValue());
+
+    if (!attributes.hasFx() && element->hasAttribute(SVGNames::fxAttr))
+        attributes.setFx(radial->fx()->currentValue());
+
+    if (!attributes.hasFy() && element->hasAttribute(SVGNames::fyAttr))
+        attributes.setFy(radial->fy()->currentValue());
+
+    if (!attributes.hasFr() && element->hasAttribute(SVGNames::frAttr))
+        attributes.setFr(radial->fr()->currentValue());
+}
+
 bool SVGRadialGradientElement::selfHasRelativeLengths() const
 {
     return m_cx->currentValue()->isRelative()
         || m_cy->currentValue()->isRelative()
         || m_r->currentValue()->isRelative()
         || m_fx->currentValue()->isRelative()
         || m_fy->currentValue()->isRelative()
         || m_fr->currentValue()->isRelative();
 }
 
 }