Simplify layout attribute invalidation in LayoutSVGText

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

Index: third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp
diff --git a/third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp b/third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp
index a5d12e49b7143fbf0b984da98180a79237c4bea8..f67174ed988d4b659356aca5a842dd8da4e52c51 100644
--- a/third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp
+++ b/third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp
@@ -112,16 +112,22 @@ inline bool LayoutSVGText::shouldHandleSubtreeMutations() const
     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()
@@ -132,31 +138,18 @@ void LayoutSVGText::willBeDestroyed()
     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());
@@ -167,9 +160,7 @@ void LayoutSVGText::subtreeTextDidChange(LayoutSVGInlineText* text)
     // 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)