[webkit-reviews] review granted: [Bug 181084] Special list-item counter starts from an incorrect number for ::before and ::after : [Attachment 330668] Patch

[bugzilla-daemon at webkit.org]

zalan <zalan at apple.com> has granted Darin Adler <darin at apple.com>'s request for
review:
Bug 181084: Special list-item counter starts from an incorrect number for
::before and ::after
https://bugs.webkit.org/show_bug.cgi?id=181084

Attachment 330668: Patch

https://bugs.webkit.org/attachment.cgi?id=330668&action=review




--- Comment #4 from zalan <zalan at apple.com> ---
Comment on attachment 330668
  --> https://bugs.webkit.org/attachment.cgi?id=330668
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330668&action=review

> Source/WebCore/rendering/RenderListItem.cpp:203
> +    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 -- we need to change this to an iterative algorithm.
> -    if (RenderListItem* previousItem = previousListItem(list, *this))
> -	   return previousItem->value() + valueStep;
> -
> -    if (oListElement)
> -	   return oListElement->start();
> +    // 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);

Couldn't you just use isInReversedOrderedList() here?

> Source/WebCore/rendering/style/CounterDirectives.h:37
> +struct CounterDirectives {
> +    std::optional<int> resetValue;
> +    std::optional<int> incrementValue;

This is a really nice cleanup (class -> struct).