Restructure LayoutSVGText::layout

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

 /*
  * Copyright (C) 2006 Apple Computer, Inc.
  * Copyright (C) 2006 Alexander Kellett <lypanov@kde.org>
  * Copyright (C) 2006 Oliver Hunt <ojh16@student.canterbury.ac.nz>
  * Copyright (C) 2007 Nikolas Zimmermann <zimmermann@kde.org>
  * Copyright (C) 2008 Rob Buis <buis@kde.org>
  * Copyright (C) 2009 Dirk Schulze <krit@webkit.org>
  * Copyright (C) Research In Motion Limited 2010-2012. All rights reserved.
  * Copyright (C) 2012 Google Inc.
  *
@@ skipping 168 matching lines @@
     collectLayoutAttributes(text, newLayoutAttributes);
     ASSERT(newLayoutAttributes == expectedLayoutAttributes);
 #endif
 }
 
 void LayoutSVGText::layout()
 {
     ASSERT(needsLayout());
     LayoutAnalyzer::Scope analyzer(*this);
 
-    bool updateCachedBoundariesInParents = false;
+    bool updateParentBoundaries = false;
     if (m_needsTransformUpdate) {
         m_localTransform = toSVGTextElement(node())->calculateAnimatedLocalTransform();
         m_needsTransformUpdate = false;
-        updateCachedBoundariesInParents = true;
+        updateParentBoundaries = true;
     }
 
+    // This flag is set and reset as needed only within this function.
+    ASSERT(!m_needsReordering);

[f(malita), 2016/04/29 17:50:54]

Nit: can we move the assert either at the beginning or closer to use location?

[fs, 2016/04/29 19:43:26]

Moved to beginning.

+
+    // When laying out initially, build the character data map and propagate
+    // resulting layout attributes to all LayoutSVGInlineText children in the
+    // subtree.
     if (!everHadLayout()) {
-        // When laying out initially, collect all layout attributes, build the character data map,
-        // and propogate resulting SVGLayoutAttributes to all LayoutSVGInlineText children in the subtree.
-        ASSERT(m_layoutAttributes.isEmpty());
-        collectLayoutAttributes(this, m_layoutAttributes);

[f(malita), 2016/04/29 17:50:54]

I guess updateFontMetrics() doesn't depend on collectLayoutAttributes()?

[fs, 2016/04/29 19:43:26]

Correct.

+        m_needsPositioningValuesUpdate = true;
+        m_needsTextMetricsUpdate = true;
+    }
+
+    // If the root layout size changed (eg. window size changes), or the screen
+    // scale factor has changed, then recompute the on-screen font size. Since
+    // the computation of layout attributes uses the text metrics, we need to
+    // update them before updating the layout attributes.
+    if (m_needsTextMetricsUpdate || SVGLayoutSupport::findTreeRootObject(this)->isLayoutSizeChanged()) {
         updateFontAndMetrics(*this);
+        m_needsTextMetricsUpdate = false;
+        updateParentBoundaries = true;
+    }
 
-        SVGTextLayoutAttributesBuilder(*this).buildLayoutAttributes();
-
-        m_needsReordering = true;
-        m_needsTextMetricsUpdate = false;
-        m_needsPositioningValuesUpdate = false;
-        updateCachedBoundariesInParents = true;
-    } else if (m_needsPositioningValuesUpdate) {
-        // When the x/y/dx/dy/rotate lists change, recompute the layout attributes, and eventually
-        // update the on-screen font objects as well in all descendants.
-        if (m_needsTextMetricsUpdate) {
-            updateFontAndMetrics(*this);
-            m_needsTextMetricsUpdate = false;
-        }
-
+    // When the x/y/dx/dy/rotate lists change, we need to recompute the layout
+    // attributes.
+    if (m_needsPositioningValuesUpdate) {
         m_layoutAttributes.clear();
         collectLayoutAttributes(this, m_layoutAttributes);
 
         SVGTextLayoutAttributesBuilder(*this).buildLayoutAttributes();
 
+        m_needsPositioningValuesUpdate = false;
         m_needsReordering = true;
-        m_needsPositioningValuesUpdate = false;
-        updateCachedBoundariesInParents = true;
-    } else if (m_needsTextMetricsUpdate || SVGLayoutSupport::findTreeRootObject(this)->isLayoutSizeChanged()) {
-        // If the root layout size changed (eg. window size changes), or the screen scale factor has
-        // changed, then recompute the on-screen font size.
-        updateFontAndMetrics(*this);
-
-        ASSERT(!m_needsReordering);
-        ASSERT(!m_needsPositioningValuesUpdate);
-        m_needsTextMetricsUpdate = false;
-        updateCachedBoundariesInParents = true;
+        updateParentBoundaries = true;

[f(malita), 2016/04/29 17:50:54]

Can we use a TemporaryChange<bool> in the outer scope to avoid an explicit reset
below?

(would have to be wrapped in Optional<>, see if it makes sense)

[fs, 2016/04/29 19:43:26]

Didn't look too shabby, but m_needsReordering is a bitfield, so TemporaryChange
won't work (since you can't take the address of a bitfield.

     }
 
     checkLayoutAttributesConsistency(this, m_layoutAttributes);
 
     // Reduced version of LayoutBlock::layoutBlock(), which only takes care of SVG text.
     // All if branches that could cause early exit in LayoutBlocks layoutBlock() method are turned into assertions.
     ASSERT(!isInline());
     ASSERT(!simplifiedLayout());
     ASSERT(!scrollsOverflow());
     ASSERT(!hasControlClip());
@@ skipping 12 matching lines @@
     LayoutUnit beforeEdge = borderBefore() + paddingBefore();
     LayoutUnit afterEdge = borderAfter() + paddingAfter() + scrollbarLogicalHeight();
     setLogicalHeight(beforeEdge);
 
     LayoutState state(*this, locationOffset());
     LayoutUnit paintInvalidationLogicalTop;
     LayoutUnit paintInvalidationLogicalBottom;
     layoutInlineChildren(true, paintInvalidationLogicalTop, paintInvalidationLogicalBottom, afterEdge);
 
     if (m_needsReordering)
         m_needsReordering = false;

[f(malita), 2016/04/29 17:50:54]

Nit: make this unconditional?

[fs, 2016/04/29 19:43:26]

Done.

 
     // If we don't have any line boxes, then make sure the frame rect is still cleared.
     if (!firstLineBox())
         setFrameRect(LayoutRect());
 
     m_overflow.clear();
     addVisualEffectOverflow();
 
-    if (!updateCachedBoundariesInParents)
-        updateCachedBoundariesInParents = oldBoundaries != objectBoundingBox();
+    if (!updateParentBoundaries)
+        updateParentBoundaries = oldBoundaries != objectBoundingBox();
 
     // Invalidate all resources of this client if our layout changed.
     if (everHadLayout() && selfNeedsLayout())
         SVGResourcesCache::clientLayoutChanged(this);
 
     // If our bounds changed, notify the parents.
-    if (updateCachedBoundariesInParents)
+    if (updateParentBoundaries)
         LayoutSVGBlock::setNeedsBoundariesUpdate();
 
+    ASSERT(!m_needsReordering);
+    ASSERT(!m_needsTransformUpdate);
+    ASSERT(!m_needsTextMetricsUpdate);
+    ASSERT(!m_needsPositioningValuesUpdate);
     clearNeedsLayout();
 }
 
 RootInlineBox* LayoutSVGText::createRootInlineBox()
 {
     RootInlineBox* box = new SVGRootInlineBox(LineLayoutItem(this));
     box->setHasVirtualLogicalHeight();
     return box;
 }
 
@@ skipping 121 matching lines @@
         paintInvalidationState.pushDelayedPaintInvalidationTarget(*this);
 
     if (reason == PaintInvalidationSVGResourceChange)
         newPaintInvalidationState.setForceSubtreeInvalidationWithinContainer();
 
     newPaintInvalidationState.updateForChildren();
     invalidatePaintOfSubtreesIfNeeded(newPaintInvalidationState);
 }
 
 } // namespace blink