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

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=119414&action=review


review- because of the mistake in HTMLOListElement::parseMappedAttribute.
Otherwise looking good.

> Source/WebCore/html/HTMLOListElement.cpp:90
> -	   if (!canParse)
> -	       start = 1;
> +	   if (!canParse) {
> +	       m_start = 1;
> +	       m_hasExplicitStart = false;
> +	   }
>	   if (start == m_start)
>	       return;
>	   m_start = start;
> -	   for (RenderObject* child = renderer(); child; child =
child->nextInPreOrder(renderer())) {
> -	       if (child->isListItem())
> -		   toRenderListItem(child)->updateValue();
> -	   }
> +	   m_hasExplicitStart = canParse;

The code in the current patch will not update item values when the start
changes from "1" to "" on a reversed list, but we need to update item values in
that case. May even need a test case.

I think the right way to write this is as follows:

    int oldStart = start();
    bool canParse;
    int parsedStart = attr->value().toInt(&canParse);
    m_hasExplicitStart = canParse;
    m_start = canParse ? parsedStart : 0xBADBEEF;
    if (oldStart == start())
	return;
    updateItemValues();

Or we could use the value 1 or 0 or 10000 rather than 0xBADBEEF.

> Source/WebCore/html/HTMLOListElement.cpp:113
> +    RenderListItem* listItem = RenderListItem::nextListItem(renderer());
> +    while (listItem) {
> +	   listItem->updateValue();
> +	   listItem = RenderListItem::nextListItem(renderer(), listItem);
> +    }

This has the structure of a for loop and I would like to write it that way;
sadly the function names are long so the for statement will be long.

> Source/WebCore/html/HTMLOListElement.cpp:119
> +unsigned HTMLOListElement::itemCount() const
> +{
> +    if (!m_shouldRecalculateItemCount)
> +	   return m_itemCount;

A cool way to write a function like this is to have an inline part:

    if (m_shouldRecalculateItemCount)
	recalculateItemCount();
    return m_itemCount;

And then a non-inline recalculateItemCount function. I also think it makes the
code read better.

> Source/WebCore/html/HTMLOListElement.cpp:127
> +    RenderListItem* listItem = RenderListItem::nextListItem(renderer());
> +    while (listItem) {
> +	   m_itemCount++;
> +	   listItem = RenderListItem::nextListItem(renderer(), listItem);
> +    }

Ditto.

> Source/WebCore/html/HTMLOListElement.cpp:137
> +void HTMLOListElement::itemCountChanged()
> +{
> +    m_shouldRecalculateItemCount = true;
> +}

Should be inlined.

> Source/WebCore/rendering/RenderListItem.cpp:468
> +    RenderListItem* item = isListReversed ? previousListItem(list, this) :
nextListItem(list, this);
> +    while (item) {

Could initialize item to list and then put the expression inside the while.
That would avoid repeating the isListReversed logic.

Or could write as a for loop if you made a helper function that does
previous/nextListItem to avoid writing out the ?: each time.