Do refactor in collectGradientAttributes() and renderStyleForLengthResolve()
authorgyuyoung.kim@samsung.com <gyuyoung.kim@samsung.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 21 Jan 2014 00:11:07 +0000 (00:11 +0000)
committergyuyoung.kim@samsung.com <gyuyoung.kim@samsung.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 21 Jan 2014 00:11:07 +0000 (00:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=126869

Reviewed by Dirk Schulze.

Some SVG functions have done a first iteration by using a boolean flag. This is
one of poor implementations. This patch fix it by extracting a logic into a new method.
Additionally it would be good to use do-while() loop instead of using while() in
renderStyleForLengthResolving() because a first condition is always true.

Merge r165358 from blink.

* svg/SVGLengthContext.cpp:
(WebCore::renderStyleForLengthResolving):
* svg/SVGLinearGradientElement.cpp:
(WebCore::setGradientAttributes):
(WebCore::SVGLinearGradientElement::collectGradientAttributes):
* svg/SVGRadialGradientElement.cpp:
(WebCore::setGradientAttributes):
(WebCore::SVGRadialGradientElement::collectGradientAttributes):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@162385 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/svg/SVGLengthContext.cpp
Source/WebCore/svg/SVGLinearGradientElement.cpp
Source/WebCore/svg/SVGRadialGradientElement.cpp

index b71250d..a0eaade 100644 (file)
@@ -1,3 +1,26 @@
+2014-01-20  Gyuyoung Kim  <gyuyoung.kim@samsung.com>
+
+        Do refactor in collectGradientAttributes() and renderStyleForLengthResolve()
+        https://bugs.webkit.org/show_bug.cgi?id=126869
+
+        Reviewed by Dirk Schulze.
+
+        Some SVG functions have done a first iteration by using a boolean flag. This is
+        one of poor implementations. This patch fix it by extracting a logic into a new method.
+        Additionally it would be good to use do-while() loop instead of using while() in
+        renderStyleForLengthResolving() because a first condition is always true.
+
+        Merge r165358 from blink.
+
+        * svg/SVGLengthContext.cpp:
+        (WebCore::renderStyleForLengthResolving):
+        * svg/SVGLinearGradientElement.cpp:
+        (WebCore::setGradientAttributes):
+        (WebCore::SVGLinearGradientElement::collectGradientAttributes):
+        * svg/SVGRadialGradientElement.cpp:
+        (WebCore::setGradientAttributes):
+        (WebCore::SVGRadialGradientElement::collectGradientAttributes):
+
 2014-01-20  Anders Carlsson  <andersca@apple.com>
 
         UserContentController should keep track of user scripts
index dc73170..07b2045 100644 (file)
@@ -204,14 +204,14 @@ float SVGLengthContext::convertValueFromPercentageToUserUnits(float value, SVGLe
 static inline RenderStyle* renderStyleForLengthResolving(const SVGElement* context)
 {
     if (!context)
-        return 0;
+        return nullptr;
 
     const ContainerNode* currentContext = context;
-    while (currentContext) {
+    do {
         if (currentContext->renderer())
             return &currentContext->renderer()->style();
         currentContext = currentContext->parentNode();
-    }
+    } while (currentContext);
 
     // There must be at least a RenderSVGRoot renderer, carrying a style.
     ASSERT_NOT_REACHED();
index 624c007..78b024f 100644 (file)
@@ -123,70 +123,75 @@ RenderPtr<RenderElement> SVGLinearGradientElement::createElementRenderer(PassRef
     return createRenderer<RenderSVGResourceLinearGradient>(*this, std::move(style));
 }
 
-bool SVGLinearGradientElement::collectGradientAttributes(LinearGradientAttributes& attributes)
+static void setGradientAttributes(SVGGradientElement& element, LinearGradientAttributes& attributes, bool isLinear = true)
 {
-    HashSet<SVGGradientElement*> processedGradients;
-
-    bool isLinear = true;
-    SVGGradientElement* current = this;
+    if (!attributes.hasSpreadMethod() && element.hasAttribute(SVGNames::spreadMethodAttr))
+        attributes.setSpreadMethod(element.spreadMethod());
 
-    while (current) {
-        if (!current->renderer())
-            return false;
+    if (!attributes.hasGradientUnits() && element.hasAttribute(SVGNames::gradientUnitsAttr))
+        attributes.setGradientUnits(element.gradientUnits());
 
-        if (!attributes.hasSpreadMethod() && current->hasAttribute(SVGNames::spreadMethodAttr))
-            attributes.setSpreadMethod(current->spreadMethod());
+    if (!attributes.hasGradientTransform() && element.hasAttribute(SVGNames::gradientTransformAttr)) {
+        AffineTransform transform;
+        element.gradientTransform().concatenate(transform);
+        attributes.setGradientTransform(transform);
+    }
 
-        if (!attributes.hasGradientUnits() && current->hasAttribute(SVGNames::gradientUnitsAttr))
-            attributes.setGradientUnits(current->gradientUnits());
+    if (!attributes.hasStops()) {
+        const Vector<Gradient::ColorStop>& stops(element.buildStops());
+        if (!stops.isEmpty())
+            attributes.setStops(stops);
+    }
 
-        if (!attributes.hasGradientTransform() && current->hasAttribute(SVGNames::gradientTransformAttr)) {
-            AffineTransform transform;
-            current->gradientTransform().concatenate(transform);
-            attributes.setGradientTransform(transform);
-        }
+    if (isLinear) {
+        SVGLinearGradientElement* linear = toSVGLinearGradientElement(&element);
 
-        if (!attributes.hasStops()) {
-            const Vector<Gradient::ColorStop>& stops(current->buildStops());
-            if (!stops.isEmpty())
-                attributes.setStops(stops);
-        }
+        if (!attributes.hasX1() && element.hasAttribute(SVGNames::x1Attr))
+            attributes.setX1(linear->x1());
 
-        if (isLinear) {
-            SVGLinearGradientElement* linear = toSVGLinearGradientElement(current);
+        if (!attributes.hasY1() && element.hasAttribute(SVGNames::y1Attr))
+            attributes.setY1(linear->y1());
 
-            if (!attributes.hasX1() && current->hasAttribute(SVGNames::x1Attr))
-                attributes.setX1(linear->x1());
+        if (!attributes.hasX2() && element.hasAttribute(SVGNames::x2Attr))
+            attributes.setX2(linear->x2());
 
-            if (!attributes.hasY1() && current->hasAttribute(SVGNames::y1Attr))
-                attributes.setY1(linear->y1());
+        if (!attributes.hasY2() && element.hasAttribute(SVGNames::y2Attr))
+            attributes.setY2(linear->y2());
+    }
+}
 
-            if (!attributes.hasX2() && current->hasAttribute(SVGNames::x2Attr))
-                attributes.setX2(linear->x2());
+bool SVGLinearGradientElement::collectGradientAttributes(LinearGradientAttributes& attributes)
+{
+    if (!renderer())
+        return false;
 
-            if (!attributes.hasY2() && current->hasAttribute(SVGNames::y2Attr))
-                attributes.setY2(linear->y2());
-        }
+    HashSet<SVGGradientElement*> processedGradients;
+    SVGGradientElement* current = this;
 
-        processedGradients.add(current);
+    setGradientAttributes(*current, attributes);
+    processedGradients.add(current);
 
+    while (true) {
         // Respect xlink:href, take attributes from referenced element
         Node* refNode = SVGURIReference::targetElementFromIRIString(current->href(), document());
-        if (refNode && (refNode->hasTagName(SVGNames::linearGradientTag) || refNode->hasTagName(SVGNames::radialGradientTag))) {
+        if (refNode && isSVGGradientElement(*refNode)) {
             current = toSVGGradientElement(refNode);
 
             // Cycle detection
-            if (processedGradients.contains(current)) {
-                current = 0;
-                break;
-            }
+            if (processedGradients.contains(current))
+                return true;
+
+            if (!current->renderer())
+                return false;
 
-            isLinear = current->hasTagName(SVGNames::linearGradientTag);
+            setGradientAttributes(*current, attributes, current->hasTagName(SVGNames::linearGradientTag));
+            processedGradients.add(current);
         } else
-            current = 0;
+            return true;
     }
 
-    return true;
+    ASSERT_NOT_REACHED();
+    return false;
 }
 
 bool SVGLinearGradientElement::selfHasRelativeLengths() const
index 5199cf7..a92bf00 100644 (file)
@@ -135,73 +135,77 @@ RenderPtr<RenderElement> SVGRadialGradientElement::createElementRenderer(PassRef
     return createRenderer<RenderSVGResourceRadialGradient>(*this, std::move(style));
 }
 
-bool SVGRadialGradientElement::collectGradientAttributes(RadialGradientAttributes& attributes)
+static void setGradientAttributes(SVGGradientElement& element, RadialGradientAttributes& attributes, bool isRadial = true)
 {
-    HashSet<SVGGradientElement*> processedGradients;
-
-    bool isRadial = true;
-    SVGGradientElement* current = this;
+    if (!attributes.hasSpreadMethod() && element.hasAttribute(SVGNames::spreadMethodAttr))
+        attributes.setSpreadMethod(element.spreadMethod());
 
-    while (current) {
-        if (!current->renderer())
-            return false;
+    if (!attributes.hasGradientUnits() && element.hasAttribute(SVGNames::gradientUnitsAttr))
+        attributes.setGradientUnits(element.gradientUnits());
 
-        if (!attributes.hasSpreadMethod() && current->hasAttribute(SVGNames::spreadMethodAttr))
-            attributes.setSpreadMethod(current->spreadMethod());
+    if (!attributes.hasGradientTransform() && element.hasAttribute(SVGNames::gradientTransformAttr)) {
+        AffineTransform transform;
+        element.gradientTransform().concatenate(transform);
+        attributes.setGradientTransform(transform);
+    }
 
-        if (!attributes.hasGradientUnits() && current->hasAttribute(SVGNames::gradientUnitsAttr))
-            attributes.setGradientUnits(current->gradientUnits());
+    if (!attributes.hasStops()) {
+        const Vector<Gradient::ColorStop>& stops(element.buildStops());
+        if (!stops.isEmpty())
+            attributes.setStops(stops);
+    }
 
-        if (!attributes.hasGradientTransform() && current->hasAttribute(SVGNames::gradientTransformAttr)) {
-            AffineTransform transform;
-            current->gradientTransform().concatenate(transform);
-            attributes.setGradientTransform(transform);
-        }
+    if (isRadial) {
+        SVGRadialGradientElement* radial = toSVGRadialGradientElement(&element);
 
-        if (!attributes.hasStops()) {
-            const Vector<Gradient::ColorStop>& stops(current->buildStops());
-            if (!stops.isEmpty())
-                attributes.setStops(stops);
-        }
+        if (!attributes.hasCx() && element.hasAttribute(SVGNames::cxAttr))
+            attributes.setCx(radial->cx());
 
-        if (isRadial) {
-            SVGRadialGradientElement* radial = toSVGRadialGradientElement(current);
+        if (!attributes.hasCy() && element.hasAttribute(SVGNames::cyAttr))
+            attributes.setCy(radial->cy());
 
-            if (!attributes.hasCx() && current->hasAttribute(SVGNames::cxAttr))
-                attributes.setCx(radial->cx());
+        if (!attributes.hasR() && element.hasAttribute(SVGNames::rAttr))
+            attributes.setR(radial->r());
 
-            if (!attributes.hasCy() && current->hasAttribute(SVGNames::cyAttr))
-                attributes.setCy(radial->cy());
+        if (!attributes.hasFx() && element.hasAttribute(SVGNames::fxAttr))
+            attributes.setFx(radial->fx());
 
-            if (!attributes.hasR() && current->hasAttribute(SVGNames::rAttr))
-                attributes.setR(radial->r());
+        if (!attributes.hasFy() && element.hasAttribute(SVGNames::fyAttr))
+            attributes.setFy(radial->fy());
 
-            if (!attributes.hasFx() && current->hasAttribute(SVGNames::fxAttr))
-                attributes.setFx(radial->fx());
+        if (!attributes.hasFr() && element.hasAttribute(SVGNames::frAttr))
+            attributes.setFr(radial->fr());
+    }
+}
 
-            if (!attributes.hasFy() && current->hasAttribute(SVGNames::fyAttr))
-                attributes.setFy(radial->fy());
+bool SVGRadialGradientElement::collectGradientAttributes(RadialGradientAttributes& attributes)
+{
+    if (!renderer())
+        return false;
 
-            if (!attributes.hasFr() && current->hasAttribute(SVGNames::frAttr))
-                attributes.setFr(radial->fr());
-        }
+    HashSet<SVGGradientElement*> processedGradients;
+    SVGGradientElement* current = this;
 
-        processedGradients.add(current);
+    setGradientAttributes(*current, attributes);
+    processedGradients.add(current);
 
+    while (true) {
         // Respect xlink:href, take attributes from referenced element
         Node* refNode = SVGURIReference::targetElementFromIRIString(current->href(), 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;
+            if (processedGradients.contains(current))
                 break;
-            }
 
-            isRadial = current->hasTagName(SVGNames::radialGradientTag);
+            if (!current->renderer())
+                return false;
+
+            setGradientAttributes(*current, attributes, current->hasTagName(SVGNames::radialGradientTag));
+            processedGradients.add(current);
         } else
-            current = 0;
+            break;
     }
 
     // Handle default values for fx/fy
@@ -210,6 +214,7 @@ bool SVGRadialGradientElement::collectGradientAttributes(RadialGradientAttribute
 
     if (!attributes.hasFy())
         attributes.setFy(attributes.cy());
+
     return true;
 }