Reduce how often LayoutSVGShape::updateShapeFromElement is called

Source/core/layout/svg/LayoutSVGEllipse.cpp

Index: Source/core/layout/svg/LayoutSVGEllipse.cpp
diff --git a/Source/core/layout/svg/LayoutSVGEllipse.cpp b/Source/core/layout/svg/LayoutSVGEllipse.cpp
index 8809a19effc56498f07bce318b1f8db40dee533e..ed8fccbeca08295363eec6e24a249e2d8a6589f4 100644
--- a/Source/core/layout/svg/LayoutSVGEllipse.cpp
+++ b/Source/core/layout/svg/LayoutSVGEllipse.cpp
@@ -47,36 +47,18 @@ LayoutSVGEllipse::~LayoutSVGEllipse()
 
 void LayoutSVGEllipse::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;
-
     calculateRadiiAndCenter();
 
-    // Spec: "A negative value is an error. A value of zero disables rendering of the element."
-    if (m_radii.width() < 0 || m_radii.height() < 0)
+    // Fall back to LayoutSVGShape and path-based hit detection if the ellipse
+    // has a non-scaling or discontinuous stroke.
+    if (hasNonScalingStroke() || !hasContinuousStroke()) {
+        LayoutSVGShape::updateShapeFromElement();
+        m_usePathFallback = true;
         return;
-
-    if (!m_radii.isEmpty()) {
-        // Fall back to LayoutSVGShape and path-based hit detection if the ellipse
-        // has a non-scaling or discontinuous stroke.
-        if (hasNonScalingStroke() || !hasContinuousStroke()) {
-            LayoutSVGShape::updateShapeFromElement();
-            m_usePathFallback = true;
-            return;
-        }
     }
 
     clearPath();
-
-    m_fillBoundingBox = FloatRect(m_center.x() - m_radii.width(), m_center.y() - m_radii.height(), 2 * m_radii.width(), 2 * m_radii.height());
-    m_strokeBoundingBox = m_fillBoundingBox;
-    if (style()->svgStyle().hasStroke())
-        m_strokeBoundingBox.inflate(strokeWidth() / 2);
 }
 
 void LayoutSVGEllipse::calculateRadiiAndCenter()
@@ -97,6 +79,32 @@ void LayoutSVGEllipse::calculateRadiiAndCenter()
     }
 }
 
+void LayoutSVGEllipse::updateStrokeAndFillBoundingBoxes()
+{
+    // Spec: "A negative value is an error."
+    if (m_radii.width() < 0 || m_radii.height() < 0) {
+        m_fillBoundingBox = FloatRect();
+        m_strokeBoundingBox = FloatRect();
+        return;
+    }
+
+    if (m_usePathFallback) {
+        // Spec: "A value of zero disables rendering of the element." so we can skip
+        // the path fallback and rely on the existing bounding box calculation.
+        if (!m_radii.isEmpty()) {
+            LayoutSVGShape::updateStrokeAndFillBoundingBoxes();
+            return;
+        }
+        m_usePathFallback = false;

[fs, 2015/06/16 10:54:02]

This is a bit unpleasant - it'd be nicer to not mutate the "geometry state" in
this method. Not a huge deal though (the path should be simple). Maybe something
to consider cleaning up later. (And in the <rect> case it's a bit more involved
because of how the geometry "reuses" the bbox for storage.)

[pdr., 2015/06/17 03:50:49]

Now that we're not rebuilding paths on all changes, it may be a perf and
complexity win to remove LayoutSVGRect and LayoutSVGEllipse entirely. WDYT?

[fs, 2015/06/17 11:15:29]

If by that you're implying only using something akin of LayoutSVGPath - and only
storing the Path - I'd be all for that. I'd have some reservations wrt to memory
usage though (ISTR that the default "blocksize" for SkPath is more than the 16
or so bytes we're currently using. There's also the question of if there'll be
overhead analysing the (Sk)Path to get a simpler primitive back (convexity etc.)
Probably worth to check with the Skia guys about that.

+        clearPath();
+    }
+
+    m_fillBoundingBox = FloatRect(m_center.x() - m_radii.width(), m_center.y() - m_radii.height(), 2 * m_radii.width(), 2 * m_radii.height());
+    m_strokeBoundingBox = m_fillBoundingBox;
+    if (style()->svgStyle().hasStroke())
+        m_strokeBoundingBox.inflate(strokeWidth() / 2);
+}
+
 bool LayoutSVGEllipse::shapeDependentStrokeContains(const FloatPoint& point)
 {
     // The optimized check below for circles does not support non-scaling or
@@ -104,8 +112,8 @@ bool LayoutSVGEllipse::shapeDependentStrokeContains(const FloatPoint& point)
     if (m_usePathFallback
         || !hasContinuousStroke()
         || m_radii.width() != m_radii.height()) {
-        if (!hasPath())
-            createPath();
+        if (!m_usePathFallback)
+            LayoutSVGShape::updateShapeFromElement();
         return LayoutSVGShape::shapeDependentStrokeContains(point);
     }