[webkit-reviews] review denied: [Bug 36724] Add support for <ol reversed> : [Attachment 51863] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jakub Wieczorek
<jwieczorek at webkit.org>'s request for review:
Bug 36724: Add support for <ol reversed>
https://bugs.webkit.org/show_bug.cgi?id=36724

Attachment 51863: patch
https://bugs.webkit.org/attachment.cgi?id=51863&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
The test has nowhere near enough coverage. Lots of the new code isn't needed at
all for the simple test case we're adding. We need code that covers all the
edge cases of the new code, especially dynamic changes of various types,
because that's the part that's easy to get wrong.

> +	   int start = attr->value().toInt(&m_hasExplicitStart);
>	   if (start == m_start)
>	       return;
>	   m_start = start;

The code above incorrectly returns early if the start changes from "start of 0"
to "no explicit start" or changes from "no explicit start" to "start of 1". We
need test cases that cover those dynamic changes, and the code needs to be
corrected.

> +	   for (RenderObject* child = renderer(); child; child =
child->nextInPreOrder(renderer())) {
> +	       if (child->isListItem())
> +		   toRenderListItem(child)->updateValue();
> +	   }

Instead of copying and pasting this code, I would prefer that we add a helper
function to do this, so it can be shared by the startAttr and reversedAttr
cases.

> +    if (m_shouldRecalcItemCount) {
> +	   unsigned itemCount = 0;
> +	   for (RenderObject* child = renderer(); child; child =
child->nextInPreOrder(renderer())) {
> +	       if (child->isListItem())
> +		   ++itemCount;
> +	   }
> +
> +	   const_cast<HTMLOListElement*>(this)->m_itemCount = itemCount;
> +	}

Rather than using const_cast, the m_itemCount member should be marked mutable.

The code above does not set m_shouldRecalcItemCount to false, which is a major
performance bug.

I believe this function will not work correctly for nested lists. You need a
test case involving nested lists, and need to correct the code to correctly
skip nested lists.

> +void HTMLOListElement::childrenChanged(bool changedByParser, Node*
beforeChange, Node* afterChange, int childCountDelta)
> +{
> +    m_shouldRecalcItemCount = true;
> +    HTMLElement::childrenChanged(changedByParser, beforeChange, afterChange,
childCountDelta);
> +}

Will this work properly if the list items are not immediate children of the
<ol> element? Do such cases exist?

> +    bool m_shouldRecalcItemCount;

Since this is used in only one or two places, I would prefer to not abbreviate
the word "recalculate" in this name.

review- because some of the problems above are serious enough that we should
not land this as-is.

This would be much better with a cross-platform test. We should add the hooks
to DumpRenderTree so we can get list item labels and make cross-platform plain
text tests for list numbering.