Further refinement to list item and counter code after "list-item" counter fix
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Jan 2018 03:24:28 +0000 (03:24 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Jan 2018 03:24:28 +0000 (03:24 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181426

Reviewed by Zalan Bujtas.

Source/WebCore:

* css/StyleBuilderCustom.h:
(WebCore::StyleBuilderCustom::applyInheritCounter): Use auto.
(WebCore::StyleBuilderCustom::applyValueCounter): Use auto, removed unneeded
null checks for things that can never be null, moved the saturated addition
here and got rid of the addIncrementValue function from CounterDirectives.
Use the saturatedAddition function from SaturatedArithmetic.h instead of the
much less efficient one that did the same thing, CounterDirectives::addClamped.

* rendering/RenderCounter.cpp:
(WebCore::listItemCounterDirectives): Use aggregate syntax for the return
statements.
(WebCore::planCounter): Changed to use a struct return value instead of two
out arguments. Use the saturatedAddition function from SaturatedArithmetic.h
instead of the much less efficient one that did the same thing,
CounterDirectives::addClamped.
(WebCore::findPlaceForCounter): Changed to use a struct return value instead
of two out arguments.
(WebCore::makeCounterNode): Updated for the above changes. Changed code to
use add instead of both get and set. Updated to keep the counter maps inside
the values of the "map of maps" instead of using a unique_ptr and allocating
each one on the heap.
(WebCore::destroyCounterNodeWithoutMapRemoval): Changed argument to a reference
instead of a pointer. Updated for changes to the map. Use RefPtr more
consistently.
(WebCore::RenderCounter::destroyCounterNodes): Use iterators less.
(WebCore::RenderCounter::destroyCounterNode): Ditto.
(WebCore::RenderCounter::rendererRemovedFromTree): Add a check of
hasCounterNodeMap here before calling destroyCounterNodes, so that function
can assume the flag is true (both other callers already check it).
(WebCore::updateCounters): Use auto and update for changes above.
(WebCore::RenderCounter::rendererStyleChanged): Use modern for loops instead
of iterators.
(showCounterRendererTree): Use auto and udpate for changes above.

* rendering/RenderListItem.cpp:
(WebCore::enclosingList): Stop referring to elements as "nodes". Changed
the local variable names for clarity.
(WebCore::nextListItemHelper): Renamed from nextListItem since it's not
intended to be called directly and we want to use a function pointer to
nextListItem. Fixed the algorithm to correctly handle ad hoc "lists" that
are not actually HTML list elements, using the definition in the enclosingList
function as the previousListItem function already did.
(WebCore::nextListItem): Updated for name changes.
(WebCore::firstListItem): Renamed from nextListItem for clarity.
(WebCore::previousListItem): Rewrote loop so it doesn't have to do things
so strangely when we find another list.
(WebCore::RenderListItem::updateItemValuesForOrderedList): Use auto and
update local variable names.
(WebCore::RenderListItem::itemCountForOrderedList): Ditto.
(WebCore::RenderListItem::updateValueNow const): Rewrote to use an iterative
algorithm instead of a recursive one. Fixes the FIXME here.
(WebCore::RenderListItem::updateValue): Use m_valueWasSetExplicitly
instead of m_explicitValue.
(WebCore::RenderListItem::explicitValueChanged): Use auto and simplified
the loop a bit.
(WebCore::RenderListItem::setExplicitValue): Set m_valueWasSetExplicitly
instead of m_explicitValue.
(WebCore::previousOrNextItem): Deleted.
(WebCore::RenderListItem::updateListMarkerNumbers): Streamlined the loop
a bit and used a fucntion pointer to handle the two different directions.
(WebCore::RenderListItem::isInReversedOrderedList const): Simplified by
getting rid of an unneeded use of pointers and local variables.

* rendering/RenderListItem.h: Use a boolean, m_valueWasSetExplicitly,
instead of a separate optional m_explicitValue.

* rendering/style/CounterDirectives.h: Since all the code in this file was
rewritten, removed old copyrights. Deleted the addIncrementValue function,
since it is clear enough in the one call site in the style builder.
Deleted the addClamped function because it was just a much slower
version of the saturatedAddition function. Made == and != into constexpr
functions since they are simple enough to be.

* rendering/style/RenderStyle.cpp:
(WebCore::RenderStyle::getCounterDirectives const): Deleted. Caller can
handle this just fine without a helper function.
* rendering/style/RenderStyle.h: Ditto.

LayoutTests:

* fast/css/counters/counter-list-item.html: Removed an extra newline at the end of
the file.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@226675 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/css/counters/counter-list-item.html
Source/WebCore/ChangeLog
Source/WebCore/css/StyleBuilderCustom.h
Source/WebCore/rendering/RenderCounter.cpp
Source/WebCore/rendering/RenderListItem.cpp
Source/WebCore/rendering/RenderListItem.h
Source/WebCore/rendering/style/CounterDirectives.h
Source/WebCore/rendering/style/RenderStyle.cpp
Source/WebCore/rendering/style/RenderStyle.h

index 19dbc5a..ce8f9ab 100644 (file)
@@ -1,3 +1,13 @@
+2018-01-09  Darin Adler  <darin@apple.com>
+
+        Further refinement to list item and counter code after "list-item" counter fix
+        https://bugs.webkit.org/show_bug.cgi?id=181426
+
+        Reviewed by Zalan Bujtas.
+
+        * fast/css/counters/counter-list-item.html: Removed an extra newline at the end of
+        the file.
+
 2018-01-09  Myles C. Maxfield  <mmaxfield@apple.com>
 
         font-display:fallback can cause a visual flash (which is supposed to be impossible)
index 4a8aa18..473e072 100644 (file)
@@ -1,3 +1,88 @@
+2018-01-09  Darin Adler  <darin@apple.com>
+
+        Further refinement to list item and counter code after "list-item" counter fix
+        https://bugs.webkit.org/show_bug.cgi?id=181426
+
+        Reviewed by Zalan Bujtas.
+
+        * css/StyleBuilderCustom.h:
+        (WebCore::StyleBuilderCustom::applyInheritCounter): Use auto.
+        (WebCore::StyleBuilderCustom::applyValueCounter): Use auto, removed unneeded
+        null checks for things that can never be null, moved the saturated addition
+        here and got rid of the addIncrementValue function from CounterDirectives.
+        Use the saturatedAddition function from SaturatedArithmetic.h instead of the
+        much less efficient one that did the same thing, CounterDirectives::addClamped.
+
+        * rendering/RenderCounter.cpp:
+        (WebCore::listItemCounterDirectives): Use aggregate syntax for the return
+        statements.
+        (WebCore::planCounter): Changed to use a struct return value instead of two
+        out arguments. Use the saturatedAddition function from SaturatedArithmetic.h
+        instead of the much less efficient one that did the same thing,
+        CounterDirectives::addClamped.
+        (WebCore::findPlaceForCounter): Changed to use a struct return value instead
+        of two out arguments.
+        (WebCore::makeCounterNode): Updated for the above changes. Changed code to
+        use add instead of both get and set. Updated to keep the counter maps inside
+        the values of the "map of maps" instead of using a unique_ptr and allocating
+        each one on the heap.
+        (WebCore::destroyCounterNodeWithoutMapRemoval): Changed argument to a reference
+        instead of a pointer. Updated for changes to the map. Use RefPtr more
+        consistently.
+        (WebCore::RenderCounter::destroyCounterNodes): Use iterators less.
+        (WebCore::RenderCounter::destroyCounterNode): Ditto.
+        (WebCore::RenderCounter::rendererRemovedFromTree): Add a check of
+        hasCounterNodeMap here before calling destroyCounterNodes, so that function
+        can assume the flag is true (both other callers already check it).
+        (WebCore::updateCounters): Use auto and update for changes above.
+        (WebCore::RenderCounter::rendererStyleChanged): Use modern for loops instead
+        of iterators.
+        (showCounterRendererTree): Use auto and udpate for changes above.
+
+        * rendering/RenderListItem.cpp:
+        (WebCore::enclosingList): Stop referring to elements as "nodes". Changed
+        the local variable names for clarity.
+        (WebCore::nextListItemHelper): Renamed from nextListItem since it's not
+        intended to be called directly and we want to use a function pointer to
+        nextListItem. Fixed the algorithm to correctly handle ad hoc "lists" that
+        are not actually HTML list elements, using the definition in the enclosingList
+        function as the previousListItem function already did.
+        (WebCore::nextListItem): Updated for name changes.
+        (WebCore::firstListItem): Renamed from nextListItem for clarity.
+        (WebCore::previousListItem): Rewrote loop so it doesn't have to do things
+        so strangely when we find another list.
+        (WebCore::RenderListItem::updateItemValuesForOrderedList): Use auto and
+        update local variable names.
+        (WebCore::RenderListItem::itemCountForOrderedList): Ditto.
+        (WebCore::RenderListItem::updateValueNow const): Rewrote to use an iterative
+        algorithm instead of a recursive one. Fixes the FIXME here.
+        (WebCore::RenderListItem::updateValue): Use m_valueWasSetExplicitly
+        instead of m_explicitValue.
+        (WebCore::RenderListItem::explicitValueChanged): Use auto and simplified
+        the loop a bit.
+        (WebCore::RenderListItem::setExplicitValue): Set m_valueWasSetExplicitly
+        instead of m_explicitValue.
+        (WebCore::previousOrNextItem): Deleted.
+        (WebCore::RenderListItem::updateListMarkerNumbers): Streamlined the loop
+        a bit and used a fucntion pointer to handle the two different directions.
+        (WebCore::RenderListItem::isInReversedOrderedList const): Simplified by
+        getting rid of an unneeded use of pointers and local variables.
+
+        * rendering/RenderListItem.h: Use a boolean, m_valueWasSetExplicitly,
+        instead of a separate optional m_explicitValue.
+
+        * rendering/style/CounterDirectives.h: Since all the code in this file was
+        rewritten, removed old copyrights. Deleted the addIncrementValue function,
+        since it is clear enough in the one call site in the style builder.
+        Deleted the addClamped function because it was just a much slower
+        version of the saturatedAddition function. Made == and != into constexpr
+        functions since they are simple enough to be.
+
+        * rendering/style/RenderStyle.cpp:
+        (WebCore::RenderStyle::getCounterDirectives const): Deleted. Caller can
+        handle this just fine without a helper function.
+        * rendering/style/RenderStyle.h: Ditto.
+
 2018-01-09  Myles C. Maxfield  <mmaxfield@apple.com>
 
         font-display:fallback can cause a visual flash (which is supposed to be impossible)
index f1a7079..07418ce 100644 (file)
@@ -1136,9 +1136,9 @@ inline void StyleBuilderCustom::applyValueWebkitTextEmphasisStyle(StyleResolver&
 template <StyleBuilderCustom::CounterBehavior counterBehavior>
 inline void StyleBuilderCustom::applyInheritCounter(StyleResolver& styleResolver)
 {
-    CounterDirectiveMap& map = styleResolver.style()->accessCounterDirectives();
+    auto& map = styleResolver.style()->accessCounterDirectives();
     for (auto& keyValue : const_cast<RenderStyle*>(styleResolver.parentStyle())->accessCounterDirectives()) {
-        CounterDirectives& directives = map.add(keyValue.key, CounterDirectives()).iterator->value;
+        auto& directives = map.add(keyValue.key, CounterDirectives { }).iterator->value;
         if (counterBehavior == Reset)
             directives.resetValue = keyValue.value.resetValue;
         else
@@ -1167,16 +1167,13 @@ inline void StyleBuilderCustom::applyValueCounter(StyleResolver& styleResolver,
 
     for (auto& item : downcast<CSSValueList>(value)) {
         Pair* pair = downcast<CSSPrimitiveValue>(item.get()).pairValue();
-        if (!pair || !pair->first() || !pair->second())
-            continue;
-
         AtomicString identifier = pair->first()->stringValue();
         int value = pair->second()->intValue();
-        CounterDirectives& directives = map.add(identifier, CounterDirectives()).iterator->value;
+        auto& directives = map.add(identifier, CounterDirectives { }).iterator->value;
         if (counterBehavior == Reset)
             directives.resetValue = value;
         else
-            directives.addIncrementValue(value);
+            directives.incrementValue = saturatedAddition(directives.incrementValue.value_or(0), value);
     }
 }
 
index 46d7c06..3f14e69 100644 (file)
@@ -46,8 +46,8 @@ using namespace HTMLNames;
 
 WTF_MAKE_ISO_ALLOCATED_IMPL(RenderCounter);
 
-typedef HashMap<AtomicString, RefPtr<CounterNode>> CounterMap;
-typedef HashMap<const RenderElement*, std::unique_ptr<CounterMap>> CounterMaps;
+using CounterMap = HashMap<AtomicString, Ref<CounterNode>>;
+using CounterMaps = HashMap<const RenderElement*, CounterMap>;
 
 static CounterNode* makeCounterNode(RenderElement&, const AtomicString& identifier, bool alwaysCreateCounter);
 
@@ -108,30 +108,34 @@ static RenderElement* nextInPreOrder(const RenderElement& renderer, const Elemen
 
 static CounterDirectives listItemCounterDirectives(RenderElement& renderer)
 {
-    CounterDirectives directives;
     if (is<RenderListItem>(renderer)) {
         auto& item = downcast<RenderListItem>(renderer);
         if (auto explicitValue = item.explicitValue())
-            directives.resetValue = explicitValue.value();
-        else
-            directives.addIncrementValue(item.isInReversedOrderedList() ? -1 : 1);
-    } else if (auto element = renderer.element()) {
+            return { *explicitValue, std::nullopt };
+        return { std::nullopt, item.isInReversedOrderedList() ? -1 : 1 };
+    }
+    if (auto element = renderer.element()) {
         if (is<HTMLOListElement>(*element)) {
             auto& list = downcast<HTMLOListElement>(*element);
-            directives.resetValue = list.start();
-            directives.addIncrementValue(list.isReversed() ? 1 : -1);
-        } else if (element->hasTagName(ulTag) || element->hasTagName(menuTag) || element->hasTagName(dirTag))
-            directives.resetValue = 0;
+            return { list.start(), list.isReversed() ? 1 : -1 };
+        }
+        if (isHTMLListElement(*element))
+            return { 0, std::nullopt };
     }
-    return directives;
+    return { };
 }
 
-static bool planCounter(RenderElement& renderer, const AtomicString& identifier, bool& isReset, int& value)
+struct CounterPlan {
+    bool isReset;
+    int value;
+};
+
+static std::optional<CounterPlan> planCounter(RenderElement& renderer, const AtomicString& identifier)
 {
     // We must have a generating node or else we cannot have a counter.
     Element* generatingElement = renderer.generatingElement();
     if (!generatingElement)
-        return false;
+        return std::nullopt;
 
     auto& style = renderer.style();
 
@@ -140,16 +144,19 @@ static bool planCounter(RenderElement& renderer, const AtomicString& identifier,
         // Sometimes elements have more then one renderer. Only the first one gets the counter
         // LayoutTests/http/tests/css/counter-crash.html
         if (generatingElement->renderer() != &renderer)
-            return false;
+            return std::nullopt;
         break;
     case BEFORE:
     case AFTER:
         break;
     default:
-        return false; // Counters are forbidden from all other pseudo elements.
+        return std::nullopt; // Counters are forbidden from all other pseudo elements.
     }
 
-    auto directives = style.getCounterDirectives(identifier);
+    CounterDirectives directives;
+
+    if (auto map = style.counterDirectives())
+        directives = map->get(identifier);
 
     if (identifier == "list-item") {
         auto itemDirectives = listItemCounterDirectives(renderer);
@@ -159,17 +166,11 @@ static bool planCounter(RenderElement& renderer, const AtomicString& identifier,
             directives.incrementValue = itemDirectives.incrementValue;
     }
 
-    if (directives.resetValue) {
-        value = CounterDirectives::addClamped(directives.resetValue.value(), directives.incrementValue.value_or(0));
-        isReset = true;
-        return true;
-    }
-    if (directives.incrementValue) {
-        value = directives.incrementValue.value();
-        isReset = false;
-        return true;
-    }
-    return false;
+    if (directives.resetValue)
+        return CounterPlan { true, saturatedAddition(*directives.resetValue, directives.incrementValue.value_or(0)) };
+    if (directives.incrementValue)
+        return CounterPlan { false, *directives.incrementValue };
+    return std::nullopt;
 }
 
 // - Finds the insertion point for the counter described by counterOwner, isReset and 
@@ -188,7 +189,12 @@ static bool planCounter(RenderElement& renderer, const AtomicString& identifier,
 // reset node.
 // - Non-reset CounterNodes cannot have descendants.
 
-static bool findPlaceForCounter(RenderElement& counterOwner, const AtomicString& identifier, bool isReset, RefPtr<CounterNode>& parent, RefPtr<CounterNode>& previousSibling)
+struct CounterInsertionPoint {
+    RefPtr<CounterNode> parent;
+    RefPtr<CounterNode> previousSibling;
+};
+
+static CounterInsertionPoint findPlaceForCounter(RenderElement& counterOwner, const AtomicString& identifier, bool isReset)
 {
     // We cannot stop searching for counters with the same identifier before we also
     // check this renderer, because it may affect the positioning in the tree of our counter.
@@ -197,48 +203,41 @@ static bool findPlaceForCounter(RenderElement& counterOwner, const AtomicString&
     // towards the begining of the document for counters with the same identifier as the one
     // we are trying to find a place for. This is the next renderer to be checked.
     RenderElement* currentRenderer = previousInPreOrder(counterOwner);
-    previousSibling = nullptr;
-    RefPtr<CounterNode> previousSiblingProtector;
+    RefPtr<CounterNode> previousSibling;
 
     while (currentRenderer) {
-        CounterNode* currentCounter = makeCounterNode(*currentRenderer, identifier, false);
+        auto currentCounter = makeCounterNode(*currentRenderer, identifier, false);
         if (searchEndRenderer == currentRenderer) {
             // We may be at the end of our search.
             if (currentCounter) {
                 // We have a suitable counter on the EndSearchRenderer.
-                if (previousSiblingProtector) { // But we already found another counter that we come after.
+                if (previousSibling) {
+                    // But we already found another counter that we come after.
                     if (currentCounter->actsAsReset()) {
                         // We found a reset counter that is on a renderer that is a sibling of ours or a parent.
                         if (isReset && areRenderersElementsSiblings(*currentRenderer, counterOwner)) {
                             // We are also a reset counter and the previous reset was on a sibling renderer
                             // hence we are the next sibling of that counter if that reset is not a root or
                             // we are a root node if that reset is a root.
-                            parent = currentCounter->parent();
-                            previousSibling = parent ? currentCounter : nullptr;
-                            return parent;
+                            auto* parent = currentCounter->parent();
+                            return { parent, parent ? currentCounter : nullptr };
                         }
                         // We are not a reset node or the previous reset must be on an ancestor of our owner renderer
                         // hence we must be a child of that reset counter.
-                        parent = currentCounter;
                         // In some cases renders can be reparented (ex. nodes inside a table but not in a column or row).
                         // In these cases the identified previousSibling will be invalid as its parent is different from
                         // our identified parent.
-                        if (previousSiblingProtector->parent() != currentCounter)
-                            previousSiblingProtector = nullptr;
-
-                        previousSibling = previousSiblingProtector.get();
-                        return true;
+                        if (previousSibling->parent() != currentCounter)
+                            previousSibling = nullptr;
+                        return { currentCounter, WTFMove(previousSibling) };
                     }
                     // CurrentCounter, the counter at the EndSearchRenderer, is not reset.
                     if (!isReset || !areRenderersElementsSiblings(*currentRenderer, counterOwner)) {
                         // If the node we are placing is not reset or we have found a counter that is attached
                         // to an ancestor of the placed counter's owner renderer we know we are a sibling of that node.
-                        if (currentCounter->parent() != previousSiblingProtector->parent())
-                            return false;
-
-                        parent = currentCounter->parent();
-                        previousSibling = previousSiblingProtector.get();
-                        return true;
+                        if (currentCounter->parent() != previousSibling->parent())
+                            return { };
+                        return { currentCounter->parent(), WTFMove(previousSibling) };
                     }
                 } else { 
                     // We are at the potential end of the search, but we had no previous sibling candidate
@@ -246,21 +245,13 @@ static bool findPlaceForCounter(RenderElement& counterOwner, const AtomicString&
                     // previousSibling, and when we are a sibling of the end counter we must set previousSibling
                     // to currentCounter.
                     if (currentCounter->actsAsReset()) {
-                        if (isReset && areRenderersElementsSiblings(*currentRenderer, counterOwner)) {
-                            parent = currentCounter->parent();
-                            previousSibling = currentCounter;
-                            return parent;
-                        }
-                        parent = currentCounter;
-                        previousSibling = previousSiblingProtector.get();
-                        return true;
-                    }
-                    if (!isReset || !areRenderersElementsSiblings(*currentRenderer, counterOwner)) {
-                        parent = currentCounter->parent();
-                        previousSibling = currentCounter;
-                        return true;
+                        if (isReset && areRenderersElementsSiblings(*currentRenderer, counterOwner))
+                            return { currentCounter->parent(), currentCounter };
+                        return { currentCounter, WTFMove(previousSibling) };
                     }
-                    previousSiblingProtector = currentCounter;
+                    if (!isReset || !areRenderersElementsSiblings(*currentRenderer, counterOwner))
+                        return { currentCounter->parent(), currentCounter };
+                    previousSibling = currentCounter;
                 }
             }
             // We come here if the previous sibling or parent of our owner renderer had no
@@ -273,18 +264,18 @@ static bool findPlaceForCounter(RenderElement& counterOwner, const AtomicString&
             // counter being placed is attached to.
             if (currentCounter) {
                 // We found a suitable counter.
-                if (previousSiblingProtector) {
+                if (previousSibling) {
                     // Since we had a suitable previous counter before, we should only consider this one as our 
                     // previousSibling if it is a reset counter and hence the current previousSibling is its child.
                     if (currentCounter->actsAsReset()) {
-                        previousSiblingProtector = currentCounter;
+                        previousSibling = currentCounter;
                         // We are no longer interested in previous siblings of the currentRenderer or their children
                         // as counters they may have attached cannot be the previous sibling of the counter we are placing.
                         currentRenderer = parentOrPseudoHostElement(*currentRenderer)->renderer();
                         continue;
                     }
                 } else
-                    previousSiblingProtector = currentCounter;
+                    previousSibling = currentCounter;
                 currentRenderer = previousSiblingOrParent(*currentRenderer);
                 continue;
             }
@@ -293,54 +284,49 @@ static bool findPlaceForCounter(RenderElement& counterOwner, const AtomicString&
         // which may be done twice in some cases. Rearranging the decision points though, to accommodate this 
         // performance improvement would create more code duplication than is worthwhile in my oppinion and may further
         // impede the readability of this already complex algorithm.
-        if (previousSiblingProtector)
+        if (previousSibling)
             currentRenderer = previousSiblingOrParent(*currentRenderer);
         else
             currentRenderer = previousInPreOrder(*currentRenderer);
     }
-    return false;
+    return { };
 }
 
 static CounterNode* makeCounterNode(RenderElement& renderer, const AtomicString& identifier, bool alwaysCreateCounter)
 {
     if (renderer.hasCounterNodeMap()) {
-        if (CounterMap* nodeMap = counterMaps().get(&renderer)) {
-            if (CounterNode* node = nodeMap->get(identifier))
-                return node;
-        }
+        ASSERT(counterMaps().contains(&renderer));
+        if (auto* node = counterMaps().find(&renderer)->value.get(identifier))
+            return node;
     }
 
-    bool isReset = false;
-    int value = 0;
-    if (!planCounter(renderer, identifier, isReset, value) && !alwaysCreateCounter)
+    auto plan = planCounter(renderer, identifier);
+    if (!plan && !alwaysCreateCounter)
         return nullptr;
 
-    RefPtr<CounterNode> newParent;
-    RefPtr<CounterNode> newPreviousSibling;
-    RefPtr<CounterNode> newNode = CounterNode::create(renderer, isReset, value);
-    if (findPlaceForCounter(renderer, identifier, isReset, newParent, newPreviousSibling))
-        newParent->insertAfter(*newNode, newPreviousSibling.get(), identifier);
-    CounterMap* nodeMap;
-    if (renderer.hasCounterNodeMap())
-        nodeMap = counterMaps().get(&renderer);
-    else {
-        nodeMap = new CounterMap;
-        counterMaps().set(&renderer, std::unique_ptr<CounterMap>(nodeMap));
-        renderer.setHasCounterNodeMap(true);
-    }
-    nodeMap->set(identifier, newNode);
+    auto& maps = counterMaps();
+
+    auto newNode = CounterNode::create(renderer, plan && plan->isReset, plan ? plan->value : 0);
+
+    auto place = findPlaceForCounter(renderer, identifier, plan && plan->isReset);
+    if (place.parent)
+        place.parent->insertAfter(newNode, place.previousSibling.get(), identifier);
+
+    maps.add(&renderer, CounterMap { }).iterator->value.add(identifier, newNode.copyRef());
+    renderer.setHasCounterNodeMap(true);
+
     if (newNode->parent())
-        return newNode.get();
-    // Checking if some nodes that were previously counter tree root nodes
-    // should become children of this node now.
-    CounterMaps& maps = counterMaps();
-    Element* stayWithin = parentOrPseudoHostElement(renderer);
-    bool skipDescendants;
-    for (RenderElement* currentRenderer = nextInPreOrder(renderer, stayWithin); currentRenderer; currentRenderer = nextInPreOrder(*currentRenderer, stayWithin, skipDescendants)) {
+        return newNode.ptr();
+
+    // Check if some nodes that were previously root nodes should become children of this node now.
+    auto* currentRenderer = &renderer;
+    auto* stayWithin = parentOrPseudoHostElement(renderer);
+    bool skipDescendants = false;
+    while ((currentRenderer = nextInPreOrder(*currentRenderer, stayWithin, skipDescendants))) {
         skipDescendants = false;
         if (!currentRenderer->hasCounterNodeMap())
             continue;
-        CounterNode* currentCounter = maps.get(currentRenderer)->get(identifier);
+        auto* currentCounter = maps.find(currentRenderer)->value.get(identifier);
         if (!currentCounter)
             continue;
         skipDescendants = true;
@@ -350,7 +336,8 @@ static CounterNode* makeCounterNode(RenderElement& renderer, const AtomicString&
             break;
         newNode->insertAfter(*currentCounter, newNode->lastChild(), identifier);
     }
-    return newNode.get();
+
+    return newNode.ptr();
 }
 
 RenderCounter::RenderCounter(Document& document, const CounterContent& counter)
@@ -443,55 +430,43 @@ void RenderCounter::computePreferredLogicalWidths(float lead)
     RenderText::computePreferredLogicalWidths(lead);
 }
 
-static void destroyCounterNodeWithoutMapRemoval(const AtomicString& identifier, CounterNode* node)
+static void destroyCounterNodeWithoutMapRemoval(const AtomicString& identifier, CounterNode& node)
 {
-    CounterNode* previous;
-    for (RefPtr<CounterNode> child = node->lastDescendant(); child && child != node; child = previous) {
+    RefPtr<CounterNode> previous;
+    for (RefPtr<CounterNode> child = node.lastDescendant(); child && child != &node; child = WTFMove(previous)) {
         previous = child->previousInPreOrder();
         child->parent()->removeChild(*child);
-        ASSERT(counterMaps().get(&child->owner())->get(identifier) == child);
-        counterMaps().get(&child->owner())->remove(identifier);
+        ASSERT(counterMaps().find(&child->owner())->value.get(identifier) == child);
+        counterMaps().find(&child->owner())->value.remove(identifier);
     }
-    if (CounterNode* parent = node->parent())
-        parent->removeChild(*node);
+    if (auto* parent = node.parent())
+        parent->removeChild(node);
 }
 
 void RenderCounter::destroyCounterNodes(RenderElement& owner)
 {
-    CounterMaps& maps = counterMaps();
-    CounterMaps::iterator mapsIterator = maps.find(&owner);
-    if (mapsIterator == maps.end())
-        return;
-    CounterMap* map = mapsIterator->value.get();
-    CounterMap::const_iterator end = map->end();
-    for (CounterMap::const_iterator it = map->begin(); it != end; ++it) {
-        destroyCounterNodeWithoutMapRemoval(it->key, it->value.get());
-    }
-    maps.remove(mapsIterator);
+    ASSERT(owner.hasCounterNodeMap());
+    auto& maps = counterMaps();
+    ASSERT(maps.contains(&owner));
+    for (auto& keyValue : maps.take(&owner))
+        destroyCounterNodeWithoutMapRemoval(keyValue.key, keyValue.value);
     owner.setHasCounterNodeMap(false);
 }
 
 void RenderCounter::destroyCounterNode(RenderElement& owner, const AtomicString& identifier)
 {
-    CounterMap* map = counterMaps().get(&owner);
-    if (!map)
+    auto map = counterMaps().find(&owner);
+    if (map == counterMaps().end())
         return;
-    CounterMap::iterator mapIterator = map->find(identifier);
-    if (mapIterator == map->end())
+    auto node = map->value.take(identifier);
+    if (!node)
         return;
-    destroyCounterNodeWithoutMapRemoval(identifier, mapIterator->value.get());
-    map->remove(mapIterator);
-    // We do not delete "map" here even if empty because we expect to reuse
-    // it soon. In order for a renderer to lose all its counters permanently,
+    destroyCounterNodeWithoutMapRemoval(identifier, *node);
+    // We do not delete the map here even if it is now empty because we expect to
+    // reuse it. In order for a renderer to lose all its counters permanently,
     // a style change for the renderer involving removal of all counter
     // directives must occur, in which case, RenderCounter::destroyCounterNodes()
-    // must be called.
-    // The destruction of the Renderer (possibly caused by the removal of its 
-    // associated DOM node) is the other case that leads to the permanent
-    // destruction of all counters attached to a Renderer. In this case
-    // RenderCounter::destroyCounterNodes() must be and is now called, too.
-    // RenderCounter::destroyCounterNodes() handles destruction of the counter
-    // map associated with a renderer, so there is no risk in leaking the map.
+    // will be called.
 }
 
 void RenderCounter::rendererRemovedFromTree(RenderElement& renderer)
@@ -502,8 +477,11 @@ void RenderCounter::rendererRemovedFromTree(RenderElement& renderer)
     if (!currentRenderer)
         currentRenderer = &renderer;
     while (true) {
-        if (is<RenderElement>(*currentRenderer))
-            destroyCounterNodes(downcast<RenderElement>(*currentRenderer));
+        if (is<RenderElement>(*currentRenderer)) {
+            auto& counterNodeRenderer = downcast<RenderElement>(*currentRenderer);
+            if (counterNodeRenderer.hasCounterNodeMap())
+                destroyCounterNodes(counterNodeRenderer);
+        }
         if (currentRenderer == &renderer)
             break;
         currentRenderer = currentRenderer->previousInPreOrder();
@@ -512,36 +490,32 @@ void RenderCounter::rendererRemovedFromTree(RenderElement& renderer)
 
 static void updateCounters(RenderElement& renderer)
 {
-    const CounterDirectiveMap* directiveMap = renderer.style().counterDirectives();
+    auto* directiveMap = renderer.style().counterDirectives();
     if (!directiveMap)
         return;
-    CounterDirectiveMap::const_iterator end = directiveMap->end();
     if (!renderer.hasCounterNodeMap()) {
-        for (CounterDirectiveMap::const_iterator it = directiveMap->begin(); it != end; ++it)
-            makeCounterNode(renderer, it->key, false);
+        for (auto& key : directiveMap->keys())
+            makeCounterNode(renderer, key, false);
         return;
     }
-    CounterMap* counterMap = counterMaps().get(&renderer);
-    ASSERT(counterMap);
-    for (CounterDirectiveMap::const_iterator it = directiveMap->begin(); it != end; ++it) {
-        RefPtr<CounterNode> node = counterMap->get(it->key);
+    ASSERT(counterMaps().contains(&renderer));
+    auto& counterMap = counterMaps().find(&renderer)->value;
+    for (auto& key : directiveMap->keys()) {
+        RefPtr<CounterNode> node = counterMap.get(key);
         if (!node) {
-            makeCounterNode(renderer, it->key, false);
+            makeCounterNode(renderer, key, false);
             continue;
         }
-        RefPtr<CounterNode> newParent;
-        RefPtr<CounterNode> newPreviousSibling;
-        
-        findPlaceForCounter(renderer, it->key, node->hasResetType(), newParent, newPreviousSibling);
-        if (node != counterMap->get(it->key))
+        auto place = findPlaceForCounter(renderer, key, node->hasResetType());
+        if (node != counterMap.get(key))
             continue;
         CounterNode* parent = node->parent();
-        if (newParent == parent && newPreviousSibling == node->previousSibling())
+        if (place.parent == parent && place.previousSibling == node->previousSibling())
             continue;
         if (parent)
             parent->removeChild(*node);
-        if (newParent)
-            newParent->insertAfter(*node, newPreviousSibling.get(), it->key);
+        if (place.parent)
+            place.parent->insertAfter(*node, place.previousSibling.get(), key);
     }
 }
 
@@ -567,40 +541,40 @@ void RenderCounter::rendererStyleChanged(RenderElement& renderer, const RenderSt
     Element* element = renderer.generatingElement();
     if (!element || !element->renderer())
         return; // cannot have generated content or if it can have, it will be handled during attaching
+
     const CounterDirectiveMap* newCounterDirectives;
     const CounterDirectiveMap* oldCounterDirectives;
     if (oldStyle && (oldCounterDirectives = oldStyle->counterDirectives())) {
         if (newStyle && (newCounterDirectives = newStyle->counterDirectives())) {
-            CounterDirectiveMap::const_iterator newMapEnd = newCounterDirectives->end();
-            CounterDirectiveMap::const_iterator oldMapEnd = oldCounterDirectives->end();
-            for (CounterDirectiveMap::const_iterator it = newCounterDirectives->begin(); it != newMapEnd; ++it) {
-                CounterDirectiveMap::const_iterator oldMapIt = oldCounterDirectives->find(it->key);
-                if (oldMapIt != oldMapEnd) {
-                    if (oldMapIt->value == it->value)
+            for (auto& keyValue : *newCounterDirectives) {
+                auto existingEntry = oldCounterDirectives->find(keyValue.key);
+                if (existingEntry != oldCounterDirectives->end()) {
+                    if (existingEntry->value == keyValue.value)
                         continue;
-                    RenderCounter::destroyCounterNode(renderer, it->key);
+                    RenderCounter::destroyCounterNode(renderer, keyValue.key);
                 }
                 // We must create this node here, because the changed node may be a node with no display such as
                 // as those created by the increment or reset directives and the re-layout that will happen will
                 // not catch the change if the node had no children.
-                makeCounterNode(renderer, it->key, false);
+                makeCounterNode(renderer, keyValue.key, false);
             }
             // Destroying old counters that do not exist in the new counterDirective map.
-            for (CounterDirectiveMap::const_iterator it = oldCounterDirectives->begin(); it !=oldMapEnd; ++it) {
-                if (!newCounterDirectives->contains(it->key))
-                    RenderCounter::destroyCounterNode(renderer, it->key);
+            for (auto& key : oldCounterDirectives->keys()) {
+                if (!newCounterDirectives->contains(key))
+                    RenderCounter::destroyCounterNode(renderer, key);
             }
         } else {
             if (renderer.hasCounterNodeMap())
                 RenderCounter::destroyCounterNodes(renderer);
         }
-    } else if (newStyle && (newCounterDirectives = newStyle->counterDirectives())) {
-        CounterDirectiveMap::const_iterator newMapEnd = newCounterDirectives->end();
-        for (CounterDirectiveMap::const_iterator it = newCounterDirectives->begin(); it != newMapEnd; ++it) {
-            // We must create this node here, because the added node may be a node with no display such as
-            // as those created by the increment or reset directives and the re-layout that will happen will
-            // not catch the change if the node had no children.
-            makeCounterNode(renderer, it->key, false);
+    } else {
+        if (newStyle && (newCounterDirectives = newStyle->counterDirectives())) {
+            for (auto& key : newCounterDirectives->keys()) {
+                // We must create this node here, because the added node may be a node with no display such as
+                // as those created by the increment or reset directives and the re-layout that will happen will
+                // not catch the change if the node had no children.
+                makeCounterNode(renderer, key, false);
+            }
         }
     }
 }
@@ -613,21 +587,21 @@ void showCounterRendererTree(const WebCore::RenderObject* renderer, const char*
 {
     if (!renderer)
         return;
-    const WebCore::RenderObject* root = renderer;
+    auto* root = renderer;
     while (root->parent())
         root = root->parent();
 
     AtomicString identifier(counterName);
-    for (const WebCore::RenderObject* current = root; current; current = current->nextInPreOrder()) {
+    for (auto* current = root; current; current = current->nextInPreOrder()) {
         if (!is<WebCore::RenderElement>(*current))
             continue;
         fprintf(stderr, "%c", (current == renderer) ? '*' : ' ');
-        for (const WebCore::RenderObject* parent = current; parent && parent != root; parent = parent->parent())
+        for (auto* ancestor = current; ancestor && ancestor != root; ancestor = ancestor->parent())
             fprintf(stderr, "    ");
         fprintf(stderr, "%p N:%p P:%p PS:%p NS:%p C:%p\n",
             current, current->node(), current->parent(), current->previousSibling(),
             current->nextSibling(), downcast<WebCore::RenderElement>(*current).hasCounterNodeMap() ?
-            counterName ? WebCore::counterMaps().get(downcast<WebCore::RenderElement>(current))->get(identifier) : (WebCore::CounterNode*)1 : (WebCore::CounterNode*)0);
+            counterName ? WebCore::counterMaps().find(downcast<WebCore::RenderElement>(current))->value.get(identifier) : (WebCore::CounterNode*)1 : (WebCore::CounterNode*)0);
     }
     fflush(stderr);
 }
index 05a8417..b354ddd 100644 (file)
 #include "HTMLUListElement.h"
 #include "InlineElementBox.h"
 #include "PseudoElement.h"
-#include "RenderChildIterator.h"
-#include "RenderInline.h"
-#include "RenderListMarker.h"
 #include "RenderView.h"
 #include "StyleInheritedData.h"
 #include <wtf/IsoMallocInlines.h>
-#if !ASSERT_DISABLED
-#include <wtf/SetForScope.h>
-#endif
 #include <wtf/StackStats.h>
 #include <wtf/StdLibExtras.h>
 
@@ -82,7 +76,6 @@ RenderStyle RenderListItem::computeMarkerStyle() const
     fontDescription.setVariantNumericSpacing(FontVariantNumericSpacing::TabularNumbers);
     parentStyle.setFontDescription(fontDescription);
     parentStyle.fontCascade().update(&document().fontSelector());
-
     if (auto markerStyle = getCachedPseudoStyle(MARKER, &parentStyle))
         return RenderStyle::clone(*markerStyle);
     auto markerStyle = RenderStyle::create();
@@ -112,82 +105,102 @@ bool isHTMLListElement(const Node& node)
 // Returns the enclosing list with respect to the DOM order.
 static Element* enclosingList(const RenderListItem& listItem)
 {
-    Element& listItemElement = listItem.element();
-    Element* parent = is<PseudoElement>(listItemElement) ? downcast<PseudoElement>(listItemElement).hostElement() : listItemElement.parentElement();
-    Element* firstNode = parent;
-    // We use parentNode because the enclosing list could be a ShadowRoot that's not Element.
-    for (; parent; parent = parent->parentElement()) {
-        if (isHTMLListElement(*parent))
-            return parent;
+    auto& element = listItem.element();
+    auto* parent = is<PseudoElement>(element) ? downcast<PseudoElement>(element).hostElement() : element.parentElement();
+    for (auto* ancestor = parent; ancestor; ancestor = ancestor->parentElement()) {
+        if (isHTMLListElement(*ancestor))
+            return ancestor;
     }
 
-    // If there's no actual <ul> or <ol> list element, then the first found
-    // node acts as our list for purposes of determining what other list items
-    // should be numbered as part of the same list.
-    return firstNode;
+    // If there's no actual list element, then the parent element acts as our
+    // list for purposes of determining what other list items should be numbered as
+    // part of the same list.
+    return parent;
 }
 
-// Returns the next list item with respect to the DOM order.
-static RenderListItem* nextListItem(const Element& listNode, const Element& element)
+static RenderListItem* nextListItemHelper(const Element& list, const Element& element)
 {
-    for (const Element* next = ElementTraversal::nextIncludingPseudo(element, &listNode); next; ) {
-        auto* renderer = next->renderer();
-        if (!renderer || isHTMLListElement(*next)) {
-            // We've found a nested, independent list or an unrendered Element : nothing to do here.
-            next = ElementTraversal::nextIncludingPseudoSkippingChildren(*next, &listNode);
+    auto* current = &element;
+    auto advance = [&] {
+        current = ElementTraversal::nextIncludingPseudo(*current, &list);
+    };
+    advance();
+    while (current) {
+        auto* renderer = current->renderer();
+        if (!is<RenderListItem>(renderer)) {
+            advance();
+            continue;
+        }
+        auto& item = downcast<RenderListItem>(*renderer);
+        auto* otherList = enclosingList(item);
+        if (!otherList) {
+            advance();
             continue;
         }
 
-        if (is<RenderListItem>(*renderer))
-            return downcast<RenderListItem>(renderer);
+        // This item is part of our current list, so it's what we're looking for.
+        if (&list == otherList)
+            return &item;
 
-        next = ElementTraversal::nextIncludingPseudo(*next, &listNode);
+        // We found ourself inside another list; skip the rest of its contents.
+        current = ElementTraversal::nextIncludingPseudoSkippingChildren(*current, &list);
     }
 
     return nullptr;
 }
 
-static inline RenderListItem* nextListItem(const Element& listNode, const RenderListItem& item)
+static inline RenderListItem* nextListItem(const Element& list, const RenderListItem& item)
 {
-    return nextListItem(listNode, item.element());
+    return nextListItemHelper(list, item.element());
 }
 
-static inline RenderListItem* nextListItem(const Element& listNode)
+static inline RenderListItem* firstListItem(const Element& list)
 {
-    return nextListItem(listNode, listNode);
+    return nextListItemHelper(list, list);
 }
 
-// Returns the previous list item with respect to the DOM order.
-static RenderListItem* previousListItem(const Element* listNode, const RenderListItem& item)
+static RenderListItem* previousListItem(const Element& list, const RenderListItem& item)
 {
-    for (const Element* current = ElementTraversal::previousIncludingPseudo(item.element(), listNode); current; current = ElementTraversal::previousIncludingPseudo(*current, listNode)) {
-        RenderElement* renderer = current->renderer();
-        if (!is<RenderListItem>(renderer))
+    auto* current = &item.element();
+    auto advance = [&] {
+        current = ElementTraversal::previousIncludingPseudo(*current, &list);
+    };
+    advance();
+    while (current) {
+        auto* renderer = current->renderer();
+        if (!is<RenderListItem>(renderer)) {
+            advance();
             continue;
-        Element* otherList = enclosingList(downcast<RenderListItem>(*renderer));
-        // This item is part of our current list, so it's what we're looking for.
-        if (listNode == otherList)
-            return downcast<RenderListItem>(renderer);
-        // 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
-        // the previousIncludingPseudo() that will be done by the loop.
-        if (otherList)
-            current = ElementTraversal::nextIncludingPseudo(*otherList);
+        }
+        auto& item = downcast<RenderListItem>(*renderer);
+        auto* otherList = enclosingList(item);
+        if (!otherList) {
+            advance();
+            continue;
+        }
+
+        // This item is part of our current list, so we found what we're looking for.
+        if (&list == otherList)
+            return &item;
+
+        // We found ourself inside another list; skip the rest of its contents by
+        // advancing to it. However, since the list itself might be a list item,
+        // don't advance past it.
+        current = otherList;
     }
     return nullptr;
 }
 
-void RenderListItem::updateItemValuesForOrderedList(const HTMLOListElement& listNode)
+void RenderListItem::updateItemValuesForOrderedList(const HTMLOListElement& list)
 {
-    for (RenderListItem* listItem = nextListItem(listNode); listItem; listItem = nextListItem(listNode, *listItem))
+    for (auto* listItem = firstListItem(list); listItem; listItem = nextListItem(list, *listItem))
         listItem->updateValue();
 }
 
-unsigned RenderListItem::itemCountForOrderedList(const HTMLOListElement& listNode)
+unsigned RenderListItem::itemCountForOrderedList(const HTMLOListElement& list)
 {
     unsigned itemCount = 0;
-    for (RenderListItem* listItem = nextListItem(listNode); listItem; listItem = nextListItem(listNode, *listItem))
+    for (auto* listItem = firstListItem(list); listItem; listItem = nextListItem(list, *listItem))
         ++itemCount;
     return itemCount;
 }
@@ -197,24 +210,33 @@ void RenderListItem::updateValueNow() const
     auto* list = enclosingList(*this);
     auto* orderedList = is<HTMLOListElement>(list) ? downcast<HTMLOListElement>(list) : nullptr;
 
-    // FIXME: This recurses to a possible depth of the length of the list.
-    // That's not good, and we can and should change this to an iterative algorithm.
-    if (auto* previousItem = previousListItem(list, *this)) {
-        m_value = previousItem->value() + (orderedList && orderedList->isReversed() ? -1 : 1);
-        return;
+    // The start item is either the closest item before this one in the list that already has a value,
+    // or the first item in the list if none have before this have values yet.
+    auto* startItem = this;
+    if (list) {
+        auto* item = this;
+        while ((item = previousListItem(*list, *item))) {
+            startItem = item;
+            if (item->m_value)
+                break;
+        }
     }
 
-    if (orderedList) {
-        m_value = orderedList->start();
-        return;
-    }
+    auto& startValue = startItem->m_value;
+    if (!startValue)
+        startValue = orderedList ? orderedList->start() : 1;
+    int value = *startValue;
+    int increment = (orderedList && orderedList->isReversed()) ? -1 : 1;
 
-    m_value = 1;
+    for (auto* item = startItem; item != this; ) {
+        item = nextListItem(*list, *item);
+        item->m_value = (value += increment);
+    }
 }
 
 void RenderListItem::updateValue()
 {
-    if (!m_explicitValue) {
+    if (!m_valueWasSetExplicitly) {
         m_value = std::nullopt;
         if (m_marker)
             m_marker->setNeedsLayoutAndPrefWidthsRecalc();
@@ -373,57 +395,53 @@ void RenderListItem::explicitValueChanged()
         m_marker->setNeedsLayoutAndPrefWidthsRecalc();
 
     updateValue();
-    Element* listNode = enclosingList(*this);
-    if (!listNode)
+    auto* list = enclosingList(*this);
+    if (!list)
         return;
-    for (RenderListItem* item = nextListItem(*listNode, *this); item; item = nextListItem(*listNode, *item))
+    auto* item = this;
+    while ((item = nextListItem(*list, *item)))
         item->updateValue();
 }
 
 void RenderListItem::setExplicitValue(std::optional<int> value)
 {
-    if (m_explicitValue == value)
-        return;
-    m_explicitValue = value;
+    if (!value) {
+        if (!m_valueWasSetExplicitly)
+            return;
+    } else {
+        if (m_valueWasSetExplicitly && m_value == value)
+            return;
+    }
+    m_valueWasSetExplicitly = value.has_value();
     m_value = value;
     explicitValueChanged();
 }
 
-static inline RenderListItem* previousOrNextItem(bool isListReversed, Element& list, RenderListItem& item)
-{
-    return isListReversed ? previousListItem(&list, item) : nextListItem(list, item);
-}
-
 void RenderListItem::updateListMarkerNumbers()
 {
-    Element* listNode = enclosingList(*this);
-    // The list node can be the shadow root which has no renderer.
-    if (!listNode)
+    auto* list = enclosingList(*this);
+    if (!list)
         return;
 
-    bool isListReversed = false;
-    if (is<HTMLOListElement>(*listNode)) {
-        HTMLOListElement& oListElement = downcast<HTMLOListElement>(*listNode);
-        oListElement.itemCountChanged();
-        isListReversed = oListElement.isReversed();
+    bool isInReversedOrderedList = false;
+    if (is<HTMLOListElement>(*list)) {
+        auto& orderedList = downcast<HTMLOListElement>(*list);
+        orderedList.itemCountChanged();
+        isInReversedOrderedList = orderedList.isReversed();
     }
-    for (RenderListItem* item = previousOrNextItem(isListReversed, *listNode, *this); item; item = previousOrNextItem(isListReversed, *listNode, *item)) {
-        if (!item->m_value) {
-            // If an item has been marked for update before, we can safely
-            // assume that all the following ones have too.
-            // This gives us the opportunity to stop here and avoid
-            // marking the same nodes again.
-            break;
-        }
+
+    // If an item has been marked for update before, we know that all following items have, too.
+    // This gives us the opportunity to stop and avoid marking the same nodes again.
+    auto* item = this;
+    auto subsequentListItem = isInReversedOrderedList ? previousListItem : nextListItem;
+    while ((item = subsequentListItem(*list, *item)) && item->m_value)
         item->updateValue();
-    }
 }
 
 bool RenderListItem::isInReversedOrderedList() const
 {
     auto* list = enclosingList(*this);
-    auto* orderedList = is<HTMLOListElement>(list) ? downcast<HTMLOListElement>(list) : nullptr;
-    return orderedList && orderedList->isReversed();
+    return is<HTMLOListElement>(list) && downcast<HTMLOListElement>(*list).isReversed();
 }
 
 } // namespace WebCore
index 110607a..e67d640 100644 (file)
@@ -40,7 +40,7 @@ public:
     int value() const;
     void updateValue();
 
-    std::optional<int> explicitValue() const { return m_explicitValue; }
+    std::optional<int> explicitValue() const { return m_valueWasSetExplicitly ? m_value : std::nullopt; }
     void setExplicitValue(std::optional<int>);
 
     void setNotInList(bool notInList) { m_notInList = notInList; }
@@ -83,9 +83,9 @@ private:
     void updateValueNow() const;
     void explicitValueChanged();
 
-    std::optional<int> m_explicitValue;
     WeakPtr<RenderListMarker> m_marker;
     mutable std::optional<int> m_value;
+    bool m_valueWasSetExplicitly { false };
     bool m_notInList { false };
 };
 
index ef0f142..4a9aade 100644 (file)
@@ -1,9 +1,5 @@
 /*
- * Copyright (C) 2000 Lars Knoll (knoll@kde.org)
- *           (C) 2000 Antti Koivisto (koivisto@kde.org)
- *           (C) 2000 Dirk Mueller (mueller@kde.org)
- * Copyright (C) 2003-2018 Apple Inc. All rights reserved.
- * Copyright (C) 2006 Graham Dennis (graham.dennis@gmail.com)
+ * Copyright (C) 2018 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
 
 #pragma once
 
-#include <memory>
 #include <wtf/HashMap.h>
-#include <wtf/MathExtras.h>
-#include <wtf/text/AtomicString.h>
 #include <wtf/text/AtomicStringHash.h>
 
 namespace WebCore {
@@ -35,19 +28,18 @@ namespace WebCore {
 struct CounterDirectives {
     std::optional<int> resetValue;
     std::optional<int> incrementValue;
-
-    void addIncrementValue(int additionalIncrement) { incrementValue = addClamped(incrementValue.value_or(0), additionalIncrement); }
-    static int addClamped(int a, int b) { return clampToInteger(static_cast<double>(a) + b);  }
 };
 
-bool operator==(const CounterDirectives&, const CounterDirectives&);
-inline bool operator!=(const CounterDirectives& a, const CounterDirectives& b) { return !(a == b); }
-
-using CounterDirectiveMap = HashMap<AtomicString, CounterDirectives>;
-
-inline bool operator==(const CounterDirectives& a, const CounterDirectives& b)
+constexpr bool operator==(const CounterDirectives& a, const CounterDirectives& b)
 {
     return a.incrementValue == b.incrementValue && a.resetValue == b.resetValue;
 }
 
+constexpr bool operator!=(const CounterDirectives& a, const CounterDirectives& b)
+{
+    return !(a == b);
+}
+
+using CounterDirectiveMap = HashMap<AtomicString, CounterDirectives>;
+
 } // namespace WebCore
index 5448184..f21eac7 100644 (file)
@@ -1356,13 +1356,6 @@ CounterDirectiveMap& RenderStyle::accessCounterDirectives()
     return *map;
 }
 
-const CounterDirectives RenderStyle::getCounterDirectives(const AtomicString& identifier) const
-{
-    if (const CounterDirectiveMap* directives = counterDirectives())
-        return directives->get(identifier);
-    return CounterDirectives();
-}
-
 const AtomicString& RenderStyle::hyphenString() const
 {
     ASSERT(hyphens() != HyphensNone);
index 58e8354..addfbd7 100644 (file)
@@ -1362,7 +1362,6 @@ public:
 
     const CounterDirectiveMap* counterDirectives() const;
     CounterDirectiveMap& accessCounterDirectives();
-    const CounterDirectives getCounterDirectives(const AtomicString& identifier) const;
 
     QuotesData* quotes() const { return m_rareInheritedData->quotes.get(); }
     void setQuotes(RefPtr<QuotesData>&&);