List marker pseudo elements.

Source/core/layout/LayoutListItem.cpp

Index: Source/core/layout/LayoutListItem.cpp
diff --git a/Source/core/layout/LayoutListItem.cpp b/Source/core/layout/LayoutListItem.cpp
index 1403114b69e6994b696544076744ff57bca58042..cee2e99f4de15a42769dd433fd3a52d9b90e7baa 100644
--- a/Source/core/layout/LayoutListItem.cpp
+++ b/Source/core/layout/LayoutListItem.cpp
@@ -25,12 +25,14 @@
 #include "core/layout/LayoutListItem.h"
 
 #include "core/HTMLNames.h"
+#include "core/dom/MarkerPseudoElement.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/TemporaryChange.h"
 #include "wtf/text/StringBuilder.h"
 
 namespace blink {
@@ -51,6 +53,9 @@ void LayoutListItem::styleDidChange(StyleDifference diff, const ComputedStyle* o
 {
     LayoutBlockFlow::styleDidChange(diff, oldStyle);
 
+    if (RuntimeEnabledFeatures::listMarkerPseudoElementEnabled())
+        return;
+
     if (style()->listStyleType() != NoneListStyle
         || (style()->listStyleImage() && !style()->listStyleImage()->errorOccurred())) {
         if (!m_marker)
@@ -74,17 +79,31 @@ void LayoutListItem::willBeDestroyed()
 void LayoutListItem::insertedIntoTree()
 {
     LayoutBlockFlow::insertedIntoTree();
-
     updateListMarkerNumbers();
 }
 
 void LayoutListItem::willBeRemovedFromTree()
 {
     LayoutBlockFlow::willBeRemovedFromTree();
-
     updateListMarkerNumbers();
 }
 
+// FIXME: This causes subtree modifications, which will set the flags and we'll
+// end up doing the check a second time next frame ....
+void LayoutListItem::handleSubtreeModifications()
+{
+    if (documentBeingDestroyed())
+        return;
+
+    if (!notifyAncestorsOfSubtreeChange())

[esprehn, 2015/04/22 07:45:46]

The tree walk should handle this for you, not the impls.

[dsinclair, 2015/04/22 20:00:39]

Done.

+        return;
+
+    if (PseudoElement* element = toElement(node())->pseudoElement(MARKER))

[esprehn, 2015/04/22 07:45:46]

Having the layout tree reach back into the DOM like this seems unfortunate.

[dsinclair, 2015/04/22 20:00:39]

I'm not sure how to fix this one? The PseudoElement knows where to insert the
layoutObject and we know that descendants have changed, so we have to ask it to
do the work.

+        toMarkerPseudoElement(element)->attachListMarkerLayoutObject();
+
+    LayoutBlockFlow::handleSubtreeModifications();

[esprehn, 2015/04/22 07:45:46]

tree walk should be external to the notification callbacks.

[dsinclair, 2015/04/22 20:00:39]

Done.

+}
+
 static bool isList(const Node& node)
 {
     return isHTMLUListElement(node) || isHTMLOListElement(node);
@@ -129,11 +148,11 @@ static LayoutListItem* nextListItem(const Node* listNode, const LayoutListItem*
             continue;
         }
 
-        LayoutObject* renderer = current->layoutObject();
-        if (renderer && renderer->isListItem())
-            return toLayoutListItem(renderer);
+        LayoutObject* layoutObject = current->layoutObject();
+        if (layoutObject && layoutObject->isListItem())
+            return toLayoutListItem(layoutObject);
 
-        // FIXME: Can this be optimized to skip the children of the elements without a renderer?
+        // FIXME: Can this be optimized to skip the children of the elements without a layoutObject?
         current = LayoutTreeBuilderTraversal::next(*current, listNode);
     }
 
@@ -146,14 +165,17 @@ static LayoutListItem* previousListItem(const Node* listNode, const LayoutListIt
     Node* current = item->node();
     ASSERT(current);
     ASSERT(!current->document().childNeedsDistributionRecalc());
-    for (current = LayoutTreeBuilderTraversal::previous(*current, listNode); current && current != listNode; current = LayoutTreeBuilderTraversal::previous(*current, listNode)) {
-        LayoutObject* renderer = current->layoutObject();
-        if (!renderer || (renderer && !renderer->isListItem()))
+    for (current = LayoutTreeBuilderTraversal::previous(*current, listNode);
+        current && current != listNode;
+        current = LayoutTreeBuilderTraversal::previous(*current, listNode)) {
+
+        LayoutObject* layoutObject = current->layoutObject();
+        if (!layoutObject || (layoutObject && !layoutObject->isListItem()))
             continue;
-        Node* otherList = enclosingList(toLayoutListItem(renderer));
+        Node* otherList = enclosingList(toLayoutListItem(layoutObject));

[esprehn, 2015/04/22 07:45:46]

I'm really not sure all this is a code improvement, it just makes it all more
verbose. Maybe layouter? or just object?

[dsinclair, 2015/04/22 20:00:39]

Ack.

         // This item is part of our current list, so it's what we're looking for.
         if (listNode == otherList)
-            return toLayoutListItem(renderer);
+            return toLayoutListItem(layoutObject);
         // We found ourself inside another list; lets skip the rest of it.
         // Use nextIncludingPseudo() here because the other list itself may actually
         // be a list item itself. We need to examine it, so we do this to counteract
@@ -213,60 +235,31 @@ void LayoutListItem::updateValueNow() const
 
 bool LayoutListItem::isEmpty() const
 {
+    if (RuntimeEnabledFeatures::listMarkerPseudoElementEnabled()) {
+        PseudoElement* element = toElement(node())->pseudoElement(MARKER);
+        return !(element && lastChild() != element->layoutObject());

[esprehn, 2015/04/22 07:45:46]

Run demorgans.

[dsinclair, 2015/04/22 20:00:39]

Done.

+    }
     return lastChild() == m_marker;
 }
 
-static LayoutObject* getParentOfFirstLineBox(LayoutBlockFlow* curr, LayoutObject* marker)
+void LayoutListItem::updateValue()
 {
-    LayoutObject* firstChild = curr->firstChild();
-    if (!firstChild)
-        return 0;
-
-    bool inQuirksMode = curr->document().inQuirksMode();
-    for (LayoutObject* currChild = firstChild; currChild; currChild = currChild->nextSibling()) {
-        if (currChild == marker)
-            continue;
-
-        if (currChild->isInline() && (!currChild->isLayoutInline() || curr->generatesLineBoxesForInlineChild(currChild)))
-            return curr;
-
-        if (currChild->isFloating() || currChild->isOutOfFlowPositioned())
-            continue;
-
-        if (!currChild->isLayoutBlockFlow() || (currChild->isBox() && toLayoutBox(currChild)->isWritingModeRoot()))
-            break;
-
-        if (curr->isListItem() && inQuirksMode && currChild->node()
-            && (isHTMLUListElement(*currChild->node()) || isHTMLOListElement(*currChild->node())))
-            break;
-
-        LayoutObject* lineBox = getParentOfFirstLineBox(toLayoutBlockFlow(currChild), marker);
-        if (lineBox)
-            return lineBox;
-    }
+    if (m_hasExplicitValue)
+        return;
 
-    return 0;
-}
+    m_isValueUpToDate = false;
 
-void LayoutListItem::updateValue()
-{
-    if (!m_hasExplicitValue) {
-        m_isValueUpToDate = false;
-        if (m_marker)
-            m_marker->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(LayoutInvalidationReason::ListValueChange);
+    if (RuntimeEnabledFeatures::listMarkerPseudoElementEnabled()) {
+        if (PseudoElement* element = toElement(node())->pseudoElement(MARKER))
+            element->layoutObject()->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(LayoutInvalidationReason::ListValueChange);
+    } else if (m_marker) {
+        m_marker->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(LayoutInvalidationReason::ListValueChange);
     }
 }
 
-static LayoutObject* firstNonMarkerChild(LayoutObject* parent)
-{
-    LayoutObject* result = parent->slowFirstChild();
-    while (result && result->isListMarker())
-        result = result->nextSibling();
-    return result;
-}
-
 void LayoutListItem::updateMarkerLocationAndInvalidateWidth()
 {
+    ASSERT(!RuntimeEnabledFeatures::listMarkerPseudoElementEnabled());
     ASSERT(m_marker);
 
     // FIXME: We should not modify the structure of the render tree
@@ -296,9 +289,11 @@ void LayoutListItem::updateMarkerLocationAndInvalidateWidth()
 
 bool LayoutListItem::updateMarkerLocation()
 {
+    ASSERT(!RuntimeEnabledFeatures::listMarkerPseudoElementEnabled());
     ASSERT(m_marker);
+
     LayoutObject* markerParent = m_marker->parent();
-    LayoutObject* lineBoxParent = getParentOfFirstLineBox(this, m_marker);
+    LayoutObject* lineBoxParent = MarkerPseudoElement::parentOfFirstLineBox(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
@@ -311,10 +306,11 @@ bool LayoutListItem::updateMarkerLocation()
 
     if (markerParent != lineBoxParent) {
         m_marker->remove();
-        lineBoxParent->addChild(m_marker, firstNonMarkerChild(lineBoxParent));
+        lineBoxParent->addChild(m_marker, MarkerPseudoElement::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())
+        if (markerParent && markerParent->isAnonymousBlock() && !toLayoutBlock(markerParent)->firstChild()
+            && !toLayoutBlock(markerParent)->continuation())
             markerParent->destroy();
         return true;
     }
@@ -326,7 +322,16 @@ void LayoutListItem::layout()
 {
     ASSERT(needsLayout());
 
-    if (m_marker) {
+    if (RuntimeEnabledFeatures::listMarkerPseudoElementEnabled()) {
+        PseudoElement* element = toElement(node())->pseudoElement(MARKER);

[esprehn, 2015/04/22 07:45:46]

I think you just want to keep m_marker, going out through the DOM to get back to
one of your descendants is kind of silly.

[dsinclair, 2015/04/22 20:00:39]

Done.

+        if (element && element->layoutObject()) {

[esprehn, 2015/04/22 07:45:46]

How does the element exist but not have a layoutObject? Earlier in updateValue()
you don't null check layoutObject.

[dsinclair, 2015/04/22 20:00:39]

Done.

+            ASSERT(element->layoutObject()->isListMarker());
+            LayoutListMarker* marker = toLayoutListMarker(element->layoutObject());
+
+            marker->updateMarginsAndContent();
+        }
+
+    } else 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.
@@ -342,23 +347,36 @@ void LayoutListItem::layout()
 void LayoutListItem::addOverflowFromChildren()
 {
     LayoutBlockFlow::addOverflowFromChildren();
-    positionListMarker();
+
+    if (RuntimeEnabledFeatures::listMarkerPseudoElementEnabled()) {
+        MarkerPseudoElement* markerElement = toMarkerPseudoElement(toElement(node())->pseudoElement(MARKER));

[esprehn, 2015/04/22 07:45:46]

I really think you want to keep m_marker and not do this. Going out through the
DOM all the time in layout is bad. We want to go the other direction with the
layout code and make it talk the DOM less (especially if we ever want this
system to be thread safe).

[dsinclair, 2015/04/22 20:00:39]

Done.

+        if (markerElement && markerElement->layoutObject()) {
+            ASSERT(markerElement->layoutObject()->isListMarker());
+            positionListMarker(toLayoutListMarker(markerElement->layoutObject()));
+        }
+    } else {
+        positionListMarker(m_marker);
+    }
 }
 
-void LayoutListItem::positionListMarker()
+void LayoutListItem::positionListMarker(LayoutListMarker* marker)
 {
-    if (m_marker && m_marker->parent()->isBox() && !m_marker->isInside() && m_marker->inlineBoxWrapper()) {
-        LayoutUnit markerOldLogicalLeft = m_marker->logicalLeft();
+    if (!marker)
+        return;
+
+    ASSERT(marker->parent());
+    if (marker->parent()->isBox() && !marker->isInside() && marker->inlineBoxWrapper()) {
+        LayoutUnit markerOldLogicalLeft = marker->logicalLeft();
         LayoutUnit blockOffset = 0;
         LayoutUnit lineOffset = 0;
-        for (LayoutBox* o = m_marker->parentBox(); o != this; o = o->parentBox()) {
+        for (LayoutBox* o = marker->parentBox(); o != this; o = o->parentBox()) {
             blockOffset += o->logicalTop();
             lineOffset += o->logicalLeft();
         }
 
         bool adjustOverflow = false;
         LayoutUnit markerLogicalLeft;
-        RootInlineBox& root = m_marker->inlineBoxWrapper()->root();
+        RootInlineBox& root = marker->inlineBoxWrapper()->root();
         bool hitSelfPaintingLayer = false;
 
         LayoutUnit lineTop = root.lineTop();
@@ -367,9 +385,9 @@ void LayoutListItem::positionListMarker()
         // FIXME: Need to account for relative positioning in the layout overflow.
         if (style()->isLeftToRightDirection()) {
             LayoutUnit leftLineOffset = logicalLeftOffsetForLine(blockOffset, logicalLeftOffsetForLine(blockOffset, false), false);
-            markerLogicalLeft = leftLineOffset - lineOffset - paddingStart() - borderStart() + m_marker->marginStart();
-            m_marker->inlineBoxWrapper()->adjustLineDirectionPosition((markerLogicalLeft - markerOldLogicalLeft).toFloat());
-            for (InlineFlowBox* box = m_marker->inlineBoxWrapper()->parent(); box; box = box->parent()) {
+            markerLogicalLeft = leftLineOffset - lineOffset - paddingStart() - borderStart() + marker->marginStart();
+            marker->inlineBoxWrapper()->adjustLineDirectionPosition((markerLogicalLeft - markerOldLogicalLeft).toFloat());
+            for (InlineFlowBox* box = marker->inlineBoxWrapper()->parent(); box; box = box->parent()) {
                 LayoutRect newLogicalVisualOverflowRect = box->logicalVisualOverflowRect(lineTop, lineBottom);
                 LayoutRect newLogicalLayoutOverflowRect = box->logicalLayoutOverflowRect(lineTop, lineBottom);
                 if (markerLogicalLeft < newLogicalVisualOverflowRect.x() && !hitSelfPaintingLayer) {
@@ -390,18 +408,18 @@ void LayoutListItem::positionListMarker()
             }
         } else {
             LayoutUnit rightLineOffset = logicalRightOffsetForLine(blockOffset, logicalRightOffsetForLine(blockOffset, false), false);
-            markerLogicalLeft = rightLineOffset - lineOffset + paddingStart() + borderStart() + m_marker->marginEnd();
-            m_marker->inlineBoxWrapper()->adjustLineDirectionPosition((markerLogicalLeft - markerOldLogicalLeft).toFloat());
-            for (InlineFlowBox* box = m_marker->inlineBoxWrapper()->parent(); box; box = box->parent()) {
+            markerLogicalLeft = rightLineOffset - lineOffset + paddingStart() + borderStart() + marker->marginEnd();
+            marker->inlineBoxWrapper()->adjustLineDirectionPosition((markerLogicalLeft - markerOldLogicalLeft).toFloat());
+            for (InlineFlowBox* box = marker->inlineBoxWrapper()->parent(); box; box = box->parent()) {
                 LayoutRect newLogicalVisualOverflowRect = box->logicalVisualOverflowRect(lineTop, lineBottom);
                 LayoutRect newLogicalLayoutOverflowRect = box->logicalLayoutOverflowRect(lineTop, lineBottom);
-                if (markerLogicalLeft + m_marker->logicalWidth() > newLogicalVisualOverflowRect.maxX() && !hitSelfPaintingLayer) {
-                    newLogicalVisualOverflowRect.setWidth(markerLogicalLeft + m_marker->logicalWidth() - newLogicalVisualOverflowRect.x());
+                if (markerLogicalLeft + marker->logicalWidth() > newLogicalVisualOverflowRect.maxX() && !hitSelfPaintingLayer) {
+                    newLogicalVisualOverflowRect.setWidth(markerLogicalLeft + marker->logicalWidth() - newLogicalVisualOverflowRect.x());
                     if (box == root)
                         adjustOverflow = true;
                 }
-                if (markerLogicalLeft + m_marker->logicalWidth() > newLogicalLayoutOverflowRect.maxX()) {
-                    newLogicalLayoutOverflowRect.setWidth(markerLogicalLeft + m_marker->logicalWidth() - newLogicalLayoutOverflowRect.x());
+                if (markerLogicalLeft + marker->logicalWidth() > newLogicalLayoutOverflowRect.maxX()) {
+                    newLogicalLayoutOverflowRect.setWidth(markerLogicalLeft + marker->logicalWidth() - newLogicalLayoutOverflowRect.x());
                     if (box == root)
                         adjustOverflow = true;
                 }
@@ -413,10 +431,10 @@ void LayoutListItem::positionListMarker()
         }
 
         if (adjustOverflow) {
-            LayoutRect markerRect(LayoutPoint(markerLogicalLeft + lineOffset, blockOffset), m_marker->size());
+            LayoutRect markerRect(LayoutPoint(markerLogicalLeft + lineOffset, blockOffset), marker->size());
             if (!style()->isHorizontalWritingMode())
                 markerRect = markerRect.transposedRect();
-            LayoutBox* o = m_marker;
+            LayoutBox* o = marker;
             bool propagateVisualOverflow = true;
             bool propagateLayoutOverflow = true;
             do {
@@ -449,15 +467,28 @@ void LayoutListItem::paint(const PaintInfo& paintInfo, const LayoutPoint& paintO
 
 const String& LayoutListItem::markerText() const
 {
-    if (m_marker)
+    if (RuntimeEnabledFeatures::listMarkerPseudoElementEnabled()) {
+        if (PseudoElement* element = toElement(node())->pseudoElement(MARKER)) {
+            if (element->layoutObject()) {
+                ASSERT(element->layoutObject()->isListMarker());
+                return toLayoutListMarker(element->layoutObject())->text();

[esprehn, 2015/04/22 07:45:45]

ditto.

[dsinclair, 2015/04/22 20:00:39]

Done.

+            }
+        }
+    } else if (m_marker) {
         return m_marker->text();
+    }
     return nullAtom.string();
 }
 
 void LayoutListItem::explicitValueChanged()
 {
-    if (m_marker)
+    if (RuntimeEnabledFeatures::listMarkerPseudoElementEnabled()) {
+        if (PseudoElement* element = toElement(node())->pseudoElement(MARKER))
+            element->layoutObject()->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(LayoutInvalidationReason::ListValueChange);

[esprehn, 2015/04/22 07:45:46]

same.

[dsinclair, 2015/04/22 20:00:39]

Done.

+    } else if (m_marker) {
         m_marker->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(LayoutInvalidationReason::ListValueChange);
+    }
+
     Node* listNode = enclosingList(this);
     for (LayoutListItem* item = this; item; item = nextListItem(listNode, item))
         item->updateValue();
@@ -489,8 +520,15 @@ void LayoutListItem::clearExplicitValue()
 void LayoutListItem::setNotInList(bool notInList)
 {
     m_notInList = notInList;
-    if (m_marker)
+    if (RuntimeEnabledFeatures::listMarkerPseudoElementEnabled()) {
+        if (PseudoElement* element = toElement(node())->pseudoElement(MARKER)) {
+            ASSERT(element->layoutObject());
+            ASSERT(element->layoutObject()->isListMarker());
+            toLayoutListMarker(element->layoutObject())->updateMarginsAndContent();

[esprehn, 2015/04/22 07:45:46]

ditto

[dsinclair, 2015/04/22 20:00:39]

Done.

+        }
+    } else if (m_marker) {
         updateMarkerLocation();
+    }
 }
 
 static LayoutListItem* previousOrNextItem(bool isListReversed, Node* list, LayoutListItem* item)