[webkit-reviews] review granted: [Bug 43166] InsertOrderedList does not switch the list type properly when it has an inner list. : [Attachment 63017] fixed mergeWithNeighboringLists

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 43166: InsertOrderedList does not switch the list type properly when it has
an inner list.
https://bugs.webkit.org/show_bug.cgi?id=43166

Attachment 63017: fixed mergeWithNeighboringLists
https://bugs.webkit.org/attachment.cgi?id=63017&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +PassRefPtr<HTMLElement>
InsertListCommand::mergeWithNeighboringLists(PassRefPtr<HTMLElement>
listElement)
> +{
> +    RefPtr<HTMLElement> listNode = listElement;

More conventional naming would be to name the PassRefPtr passedListElement and
name the local variable listElement. But given the context, I would use the
name passedList and list. In this function everything is an element and there's
no need to use the word element.

> +	   return listNode;

It's slightly more efficient if you return listNode.release() here.

> +    return listNode;

It's slightly more efficient if you return listNode.release() here.

> +	       RefPtr<HTMLElement> listElement = createHTMLElement(document(),
listTag);
> +	       insertNodeBefore(listElement, listNode);

The local variable names here are not all that good. Here, listNode is an
enclosing HTML list element and listElement is a newly created HTML list
element. Both are lists, both are elements, both are nodes, so it's confusing
to have one named listNode and the other named listElement. It's like naming
one variable startNumber and another variable startInteger when they contain
two distinct values.