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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 119485: Patch
https://bugs.webkit.org/attachment.cgi?id=119485&action=review

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


> Source/WebCore/html/HTMLOListElement.h:50
> +	       const_cast<HTMLOListElement*>(this)->recalculateItemCount();

Why did you switch to const_cast rather than mutable? I thought earlier
versions of the patch used mutable instead.

> Source/WebCore/rendering/RenderListItem.cpp:122
> +    RenderObject* renderer = item ? item->nextInPreOrder(list) :
list->nextInPreOrder(list);
> +
> +    while (renderer) {
> +	   if (renderer->node() && isList(renderer->node())) {
> +	       // We've found a nested, independent list: nothing to do here.
> +	       renderer = renderer->nextInPreOrderAfterChildren(list);
> +	       continue;
> +	   }
> +
> +	   if (renderer->isListItem())
> +	       return toRenderListItem(renderer);
> +
> +	   renderer = renderer->nextInPreOrder(list);
> +    }

We can tighten up this loop:

    RenderObject* renderer = item ? item : list;
    while ((renderer = renderer->nextInPreOrder(list)))

I think that reads better than repeating the nextInPreOrder before and at the
end of the loop.

> Source/WebCore/rendering/RenderListItem.h:52
> +    static RenderListItem* nextListItem(RenderObject*, const RenderListItem*
= 0);

The first argument here needs an argument name. It’s not at all obvious that
it’s the list’s renderer. It would be easy to make the mistake of passing a
RenderListItem instead.