Simplify layout attribute invalidation in LayoutSVGText

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 94 matching lines @@
 inline bool LayoutSVGText::shouldHandleSubtreeMutations() const
 {
     if (beingDestroyed() || !everHadLayout()) {
         ASSERT(m_layoutAttributes.isEmpty());
         ASSERT(!m_layoutAttributesBuilder.numberOfTextPositioningElements());
         return false;
     }
     return true;
 }
 
+void LayoutSVGText::invalidatePositioningValues(LayoutInvalidationReasonForTracing reason)
+{
+    m_layoutAttributes.clear();
+    m_layoutAttributesBuilder.clearTextPositioningElements();
+    setNeedsPositioningValuesUpdate();
+    setNeedsLayoutAndFullPaintInvalidation(reason);
+}
+
 void LayoutSVGText::subtreeChildWasAdded(LayoutObject*)
 {
     if (!shouldHandleSubtreeMutations() || documentBeingDestroyed())

[pdr., 2016/04/05 18:12:36]

(Minor cleanup for some other patch) shouldHandleSubtreeMutations is actually
just "beingDestroyed() || !everHadLayout()". I think it would make more sense to
just write those conditions, or rename the function and move
documentBeingDestroyed into shouldHandleSubtreeMutations.

[fs, 2016/04/05 19:12:17]

I folded shouldHandleSubtreeMutations() into its two callers. (Kept the asserts
around although I'm not convinced that they are that useful.)

         return;
 
-    // The positioning elements cache doesn't include the new 'child' yet. Clear the
-    // cache, as the next buildLayoutAttributesForText() call rebuilds it.
-    m_layoutAttributesBuilder.clearTextPositioningElements();
-    setNeedsPositioningValuesUpdate();
-    setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::ChildChanged);
+    // The positioning elements cache depends on the size of each text layoutObject in the
+    // subtree. If this changes, clear the cache. It will be rebuilt on the next layout.
+    invalidatePositioningValues(LayoutInvalidationReason::ChildChanged);
 }
 
 void LayoutSVGText::willBeDestroyed()
 {
     m_layoutAttributes.clear();
     m_layoutAttributesBuilder.clearTextPositioningElements();
 
     LayoutSVGBlock::willBeDestroyed();
 }
 
-void LayoutSVGText::subtreeChildWillBeRemoved(LayoutObject* child)
+void LayoutSVGText::subtreeChildWillBeRemoved(LayoutObject*)

[pdr., 2016/04/05 18:12:36]

Similar here, can you remove the argument entirely?

 {
-    ASSERT(child);
     if (!shouldHandleSubtreeMutations())
         return;
 
     // The positioning elements cache depends on the size of each text layoutObject in the
-    // subtree. If this changes, clear the cache. It will be rebuilt below on the next layout.
-    m_layoutAttributesBuilder.clearTextPositioningElements();
-    setNeedsPositioningValuesUpdate();
-    setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::ChildChanged);
-
-    if (m_layoutAttributes.isEmpty() || !child->isSVGInlineText())
-        return;
-
-    // Make sure that a text node (layout attribute) reference is not left
-    // dangling in |m_layoutAttributes|.
-    size_t position = m_layoutAttributes.find(toLayoutSVGInlineText(child)->layoutAttributes());
-    ASSERT(position != kNotFound);
-    m_layoutAttributes.remove(position);
+    // subtree. If this changes, clear the cache. It will be rebuilt on the next layout.
+    invalidatePositioningValues(LayoutInvalidationReason::ChildChanged);
 }
 
-void LayoutSVGText::subtreeTextDidChange(LayoutSVGInlineText* text)
+void LayoutSVGText::subtreeTextDidChange(LayoutSVGInlineText*)

[pdr., 2016/04/05 18:12:36]

LayoutSVGInlineText::setTextInternal is the only callsite for this. Can you
remove this argument entirely?

[fs, 2016/04/05 19:12:17]

Removed dead argument in all (three) cases.

 {
-    ASSERT(text);
     ASSERT(!beingDestroyed());
     if (!everHadLayout()) {
         ASSERT(m_layoutAttributes.isEmpty());
         ASSERT(!m_layoutAttributesBuilder.numberOfTextPositioningElements());
         return;
     }
 
     // The positioning elements cache depends on the size of each text object in
     // the subtree. If this changes, clear the cache and mark it for rebuilding
     // in the next layout.
-    m_layoutAttributesBuilder.clearTextPositioningElements();
-    setNeedsPositioningValuesUpdate();
-    setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::TextChanged);
+    invalidatePositioningValues(LayoutInvalidationReason::TextChanged);
 }
 
 static inline void updateFontInAllDescendants(LayoutSVGText& textRoot, SVGTextLayoutAttributesBuilder* builder = nullptr)
 {
     for (LayoutObject* descendant = &textRoot; descendant; descendant = descendant->nextInPreOrder(&textRoot)) {
         if (!descendant->isSVGInlineText())
             continue;
         LayoutSVGInlineText* text = toLayoutSVGInlineText(descendant);
         text->updateScaledFont();
         if (builder)
@@ skipping 233 matching lines @@
         paintInvalidationState.pushDelayedPaintInvalidationTarget(*this);
 
     if (reason == PaintInvalidationSVGResourceChange)
         newPaintInvalidationState.setForceSubtreeInvalidationWithinContainer();
 
     newPaintInvalidationState.updateForChildren();
     invalidatePaintOfSubtreesIfNeeded(newPaintInvalidationState);
 }
 
 } // namespace blink