Fix getBBox() returning (0,0) bug when width or height is zero

Source/core/rendering/svg/RenderSVGEllipse.cpp

 /*
  * Copyright (C) 2012 Google, Inc.
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 35 matching lines @@
 
 void RenderSVGEllipse::updateShapeFromElement()
 {
     // Before creating a new object we need to clear the cached bounding box
     // to avoid using garbage.
     m_fillBoundingBox = FloatRect();
     m_strokeBoundingBox = FloatRect();
     m_center = FloatPoint();
     m_radii = FloatSize();
 
+    m_usePathFallback = false;
     // Fallback to RenderSVGShape if shape has a non-scaling stroke.
     if (hasNonScalingStroke()) {
         RenderSVGShape::updateShapeFromElement();
         m_usePathFallback = true;

[pdr., 2014/03/24 17:39:32]

I'm afraid this part isn't right.

Some background: RenderSVGShape is the superclass of the renderers for path,
rect, ellipse, etc. RenderSVGShape knows how to render a path but it's wasteful
in some cases (e.g., a rect) so we have specialized renderers for the simple
cases. When the specialized renderers can't handle some quirk such as non
scaling stroke, they fall back to the generic but slow RenderSVGShape.

With this change we are mixing the fallback and specialized cases.
calculateRadiiAndCenter is for the specialized ellipse code and shouldn't be
needed in the fallback case.



I suspect this explains why so many test results were affected. I don't think we
have that many shapes with width=0 or height=0 in our tests :)

[jmunhoz, 2014/03/24 18:51:24]

On 2014/03/24 17:39:32, pdr wrote:
[...] 
I understand. I will redo the patch. Thanks!
:)

-        return;
-    } else
-        m_usePathFallback = false;
+    }
 
     calculateRadiiAndCenter();
 
-    // Spec: "A value of zero disables rendering of the element."
-    if (m_radii.width() <= 0 || m_radii.height() <= 0)
+    // Spec: "A negative value is an error. A value of zero disables rendering of the element."
+    if (m_radii.isZero() || m_radii.width() < 0 || m_radii.height() < 0)
         return;
 
     m_fillBoundingBox = FloatRect(m_center.x() - m_radii.width(), m_center.y() - m_radii.height(), 2 * m_radii.width(), 2 * m_radii.height());
+
+    if (m_usePathFallback)
+        return;
+
     m_strokeBoundingBox = m_fillBoundingBox;
-    if (style()->svgStyle()->hasStroke())
+    if (style()->svgStyle()->hasStroke() && m_radii.width() && m_radii.height());

[Stephen Chennney, 2014/03/24 15:49:16]

Typo? Does strokeWidth() depend on having non-zero radii? Otherwise, why do you
want the new check at all?

         m_strokeBoundingBox.inflate(strokeWidth() / 2);
 }
 
 void RenderSVGEllipse::calculateRadiiAndCenter()
 {
     ASSERT(element());
     if (isSVGCircleElement(*element())) {
         SVGCircleElement& circle = toSVGCircleElement(*element());
 
         SVGLengthContext lengthContext(&circle);
@@ skipping 63 matching lines @@
     FloatPoint center = FloatPoint(m_center.x() - point.x(), m_center.y() - point.y());
 
     // This works by checking if the point satisfies the ellipse equation.
     // (x/rX)^2 + (y/rY)^2 <= 1
     float xrX = center.x() / m_radii.width();
     float yrY = center.y() / m_radii.height();
     return xrX * xrX + yrY * yrY <= 1.0;
 }
 
 }