[Webkit-unassigned] [Bug 43166] InsertOrderedList does not switch the list type properly when it has an inner list.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43166


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #63017|review?                     |review+
               Flag|                            |




--- Comment #8 from Darin Adler <darin at apple.com>  2010-07-29 20:03:32 PST ---
(From update of attachment 63017)
> +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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.