Update list markers in notify change.

Source/core/layout/LayoutListItem.cpp

 /**
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Andrew Wellington (proton@wiretapped.net)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
@@ skipping 11 matching lines @@
  */
 
 #include "config.h"
 #include "core/layout/LayoutListItem.h"
 
 #include "core/HTMLNames.h"
 #include "core/dom/shadow/ComposedTreeTraversal.h"
 #include "core/html/HTMLOListElement.h"
 #include "core/layout/LayoutListMarker.h"
 #include "core/layout/LayoutView.h"
-#include "core/layout/TextAutosizer.h"
 #include "wtf/StdLibExtras.h"
 #include "wtf/text/StringBuilder.h"
 
 namespace blink {
 
 using namespace HTMLNames;
 
 LayoutListItem::LayoutListItem(Element* element)
     : LayoutBlockFlow(element)
     , m_marker(nullptr)
     , m_hasExplicitValue(false)
     , m_isValueUpToDate(false)
     , m_notInList(false)
 {
     setInline(false);
+    setConsumesSubtreeChangeNotification();
+    setAncestorNeedsSubtreeChangeNotification(true);
+    setNeedsSubtreeChangeNotification();
 }
 
 void LayoutListItem::styleDidChange(StyleDifference diff, const ComputedStyle* oldStyle)
 {
     LayoutBlockFlow::styleDidChange(diff, oldStyle);
 
     if (style()->listStyleType() != NoneListStyle
         || (style()->listStyleImage() && !style()->listStyleImage()->errorOccurred())) {
         if (!m_marker)
             m_marker = LayoutListMarker::createAnonymous(this);
         m_marker->listItemStyleDidChange();
+        setNeedsSubtreeChangeNotification();
     } else if (m_marker) {
         m_marker->destroy();
         m_marker = nullptr;
     }
 }
 
 void LayoutListItem::willBeDestroyed()
 {
     if (m_marker) {
         m_marker->destroy();
         m_marker = nullptr;
     }
     LayoutBlockFlow::willBeDestroyed();
 }
 
 void LayoutListItem::insertedIntoTree()
 {
     LayoutBlockFlow::insertedIntoTree();
 
     updateListMarkerNumbers();
 }
 
 void LayoutListItem::willBeRemovedFromTree()
 {
     LayoutBlockFlow::willBeRemovedFromTree();
 
     updateListMarkerNumbers();
 }
 
+void LayoutListItem::subtreeDidChange()
+{
+    if (!m_marker)
+        return;
+
+    if (!updateMarkerLocation())
+        return;
+
+    // If the marker is inside we need to redo the preferred width calculations
+    // as the size of the item now includes the size of the list marker.
+    if (m_marker->isInside())
+        containingBlock()->setPreferredLogicalWidthsDirty();

[ojan, 2015/04/30 02:56:45]

I'm not 100% sure, but I think you want to setPreferredLogicalWidthsDirty() on
the LayoutListItem itself since it's the preferred width of the list item itself
that changes. That will mark up the tree appropriately.

[dsinclair, 2015/04/30 17:52:29]

Done. This was copied from the original code which used containingBlock instead
of the ListItem.

+}
+
 static bool isList(const Node& node)
 {
     return isHTMLUListElement(node) || isHTMLOListElement(node);
 }
 
 // Returns the enclosing list with respect to the DOM order.
 static Node* enclosingList(const LayoutListItem* listItem)
 {
     Node* listItemNode = listItem->node();
     if (!listItemNode)
@@ skipping 160 matching lines @@
 }
 
 static LayoutObject* firstNonMarkerChild(LayoutObject* parent)
 {
     LayoutObject* result = parent->slowFirstChild();
     while (result && result->isListMarker())
         result = result->nextSibling();
     return result;
 }
 
-void LayoutListItem::updateMarkerLocationAndInvalidateWidth()
-{
-    ASSERT(m_marker);
-
-    // FIXME: We should not modify the structure of the render tree
-    // during layout. crbug.com/370461
-    DeprecatedDisableModifyLayoutTreeStructureAsserts disabler;
-    LayoutState* layoutState = view()->layoutState();
-    LayoutFlowThread* currentFlowThread = nullptr;
-    if (layoutState) {
-        // We're about to modify the layout tree structure (during layout!), and any code using
-        // LayoutState might get utterly confused by that. There's no evidence that anything other
-        // than the flow thread code will suffer, though, so just reset the current flow thread
-        // temporarily.
-        // FIXME: get rid of this hack, including the flow thread setter in LayoutState, as part of
-        // fixing crbug.com/370461
-        currentFlowThread = layoutState->flowThread();
-        layoutState->setFlowThread(nullptr);
-    }
-    if (updateMarkerLocation()) {
-        // If the marker is inside we need to redo the preferred width calculations
-        // as the size of the item now includes the size of the list marker.
-        if (m_marker->isInside())
-            containingBlock()->updateLogicalWidth();
-    }
-    if (layoutState)
-        layoutState->setFlowThread(currentFlowThread);
-}
-
 bool LayoutListItem::updateMarkerLocation()
 {
     ASSERT(m_marker);
+
     LayoutObject* markerParent = m_marker->parent();
     LayoutObject* lineBoxParent = getParentOfFirstLineBox(this, m_marker);
     if (!lineBoxParent) {
         // If the marker is currently contained inside an anonymous box, then we
         // are the only item in that anonymous box (since no line box parent was
         // found). It's ok to just leave the marker where it is in this case.
         if (markerParent && markerParent->isAnonymousBlock())
             lineBoxParent = markerParent;
         else
             lineBoxParent = this;
     }
 
     if (markerParent != lineBoxParent) {
         m_marker->remove();
         lineBoxParent->addChild(m_marker, firstNonMarkerChild(lineBoxParent));
         m_marker->updateMarginsAndContent();
         // If markerParent is an anonymous block with no children, destroy it.
         if (markerParent && markerParent->isAnonymousBlock() && !toLayoutBlock(markerParent)->firstChild() && !toLayoutBlock(markerParent)->continuation())
             markerParent->destroy();
         return true;
     }
 
     return false;
 }
 
-void LayoutListItem::layout()
-{
-    ASSERT(needsLayout());
-
-    if (m_marker) {
-        // The marker must be autosized before calling
-        // updateMarkerLocationAndInvalidateWidth. It cannot be done in the
-        // parent's beginLayout because it is not yet in the render tree.
-        if (TextAutosizer* textAutosizer = document().textAutosizer())
-            textAutosizer->inflateListItem(this, m_marker);
-
-        updateMarkerLocationAndInvalidateWidth();
-    }
-
-    LayoutBlockFlow::layout();
-}
-
 void LayoutListItem::addOverflowFromChildren()
 {
     LayoutBlockFlow::addOverflowFromChildren();
     positionListMarker();
 }
 
 void LayoutListItem::positionListMarker()
 {
     if (m_marker && m_marker->parent()->isBox() && !m_marker->isInside() && m_marker->inlineBoxWrapper()) {
         LayoutUnit markerOldLogicalLeft = m_marker->logicalLeft();
@@ skipping 130 matching lines @@
     if (!m_hasExplicitValue)
         return;
     m_hasExplicitValue = false;
     m_isValueUpToDate = false;
     explicitValueChanged();
 }
 
 void LayoutListItem::setNotInList(bool notInList)
 {
     m_notInList = notInList;
-    if (m_marker)
-        updateMarkerLocation();
+    setNeedsSubtreeChangeNotification();

[ojan, 2015/04/30 02:56:45]

Since we set this in the constructor, do we need to call this here? I only see
one caller of this function and that's inside attach, where were should have
just created the LayoutListItem.

[dsinclair, 2015/04/30 17:52:30]

Good catch. Done.

 }
 
 static LayoutListItem* previousOrNextItem(bool isListReversed, Node* list, LayoutListItem* item)
 {
     return isListReversed ? previousListItem(list, item) : nextListItem(list, item);
 }
 
 void LayoutListItem::updateListMarkerNumbers()
 {
     // If distribution recalc is needed, updateListMarkerNumber will be re-invoked
@@ skipping 24 matching lines @@
             // assume that all the following ones have too.
             // This gives us the opportunity to stop here and avoid
             // marking the same nodes again.
             break;
         }
         item->updateValue();
     }
 }
 
 } // namespace blink