Don't use a SubtreeLayoutScope object for SVG resource layout

third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp

Index: third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp
diff --git a/third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp b/third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp
index ae891e952ff47c2cedc1e46039b703a561cd24e5..65864c7721cc15faf72d18e9122b69d7675e1473 100644
--- a/third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp
+++ b/third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp
@@ -259,6 +259,46 @@ bool SVGLayoutSupport::transformToRootChanged(LayoutObject* ancestor)
     return false;
 }
 
+void layoutChild(LayoutObject* child, bool forceLayout, bool layoutSizeChanged, bool transformChanged, bool isResourceContainer)

[f(malita), 2016/03/02 18:24:50]

static or anonymous namespace

[fs, 2016/03/02 19:13:24]

Nit: Make this local to the compilation unit.

+{
+    if (transformChanged) {
+        // If the transform changed we need to update the text metrics (note: this also happens for layoutSizeChanged=true).
+        if (child->isSVGText())
+            toLayoutSVGText(child)->setNeedsTextMetricsUpdate();
+        forceLayout = true;
+    }
+
+    if (layoutSizeChanged) {
+        // When selfNeedsLayout is false and the layout size changed, we have to check whether this child uses relative lengths
+        if (SVGElement* element = child->node()->isSVGElement() ? toSVGElement(child->node()) : 0) {
+            if (element->hasRelativeLengths()) {
+                // FIXME: this should be done on invalidation, not during layout.
+                // When the layout size changed and when using relative values tell the LayoutSVGShape to update its shape object
+                if (child->isSVGShape()) {
+                    toLayoutSVGShape(child)->setNeedsShapeUpdate();
+                } else if (child->isSVGText()) {
+                    toLayoutSVGText(child)->setNeedsTextMetricsUpdate();
+                    toLayoutSVGText(child)->setNeedsPositioningValuesUpdate();
+                }
+
+                forceLayout = true;
+            }
+        }
+    }
+
+    SubtreeLayoutScope layoutScope(*child);
+    // Resource containers are nasty: they can invalidate clients outside the current SubtreeLayoutScope.
+    // Since they only care about viewport size changes (to resolve their relative lengths), we trigger
+    // their invalidation directly from SVGSVGElement::svgAttributeChange() or at a higher
+    // SubtreeLayoutScope (in LayoutView::layout()).
+    if (forceLayout && !isResourceContainer)
+        layoutScope.setNeedsLayout(child, LayoutInvalidationReason::SvgChanged);
+
+    // Lay out any referenced resources before the child.
+    SVGLayoutSupport::layoutResourcesIfNeeded(child);
+    child->layoutIfNeeded();
+}
+
 void SVGLayoutSupport::layoutChildren(LayoutObject* start, bool selfNeedsLayout)
 {
     // When hasRelativeLengths() is false, no descendants have relative lengths
@@ -268,44 +308,13 @@ void SVGLayoutSupport::layoutChildren(LayoutObject* start, bool selfNeedsLayout)
     bool transformChanged = transformToRootChanged(start);
 
     for (LayoutObject* child = start->slowFirstChild(); child; child = child->nextSibling()) {

[f(malita), 2016/03/02 18:24:50]

Cache the start child and reuse below (to avoid two slowFirstChild calls)?

-        bool forceLayout = selfNeedsLayout;
-
-        if (transformChanged) {
-            // If the transform changed we need to update the text metrics (note: this also happens for layoutSizeChanged=true).
-            if (child->isSVGText())
-                toLayoutSVGText(child)->setNeedsTextMetricsUpdate();
-            forceLayout = true;
-        }
-
-        if (layoutSizeChanged) {
-            // When selfNeedsLayout is false and the layout size changed, we have to check whether this child uses relative lengths
-            if (SVGElement* element = child->node()->isSVGElement() ? toSVGElement(child->node()) : 0) {
-                if (element->hasRelativeLengths()) {
-                    // FIXME: this should be done on invalidation, not during layout.
-                    // When the layout size changed and when using relative values tell the LayoutSVGShape to update its shape object
-                    if (child->isSVGShape()) {
-                        toLayoutSVGShape(child)->setNeedsShapeUpdate();
-                    } else if (child->isSVGText()) {
-                        toLayoutSVGText(child)->setNeedsTextMetricsUpdate();
-                        toLayoutSVGText(child)->setNeedsPositioningValuesUpdate();
-                    }
-
-                    forceLayout = true;
-                }
-            }
-        }
+        if (child->isSVGResourceContainer())
+            layoutChild(child, selfNeedsLayout, layoutSizeChanged, transformChanged, true);

[fs, 2016/03/02 19:13:24]

I'm not sure we need to have this call layoutChild (it can rather be confusing),
since it will in practice do very little - child->isSVGText() and
child->isSVGShape() will be false and forceLayout itself will be cancelled out
by isResourceContainer==true. So we're left with:

    SubtreeLayoutScope layoutScope(*child);
    SVGLayoutSupport::layoutResourcesIfNeeded(child);
    child->layoutIfNeeded();

for the resource-container case (and the scope is only for the assert in the
constructor I think.)

So maybe it would make sense to recognize that the two cases are different
enough that using the same function might be more confusing than not? So keep
the old loop unchanged (adding the !isResourceContainer branch) and add a new
loop that just does the simple obvious stuff? If not, then  at least add a
comment explaining stuff.

+    }
 
-        SubtreeLayoutScope layoutScope(*child);
-        // Resource containers are nasty: they can invalidate clients outside the current SubtreeLayoutScope.
-        // Since they only care about viewport size changes (to resolve their relative lengths), we trigger
-        // their invalidation directly from SVGSVGElement::svgAttributeChange() or at a higher
-        // SubtreeLayoutScope (in LayoutView::layout()).
-        if (forceLayout && !child->isSVGResourceContainer())
-            layoutScope.setNeedsLayout(child, LayoutInvalidationReason::SvgChanged);
-
-        // Lay out any referenced resources before the child.
-        layoutResourcesIfNeeded(child);
-        child->layoutIfNeeded();
+    for (LayoutObject* child = start->slowFirstChild(); child; child = child->nextSibling()) {
+        if (!child->isSVGResourceContainer())
+            layoutChild(child, selfNeedsLayout, layoutSizeChanged, transformChanged, false);
     }
 }