[webkit-reviews] review granted: [Bug 37060] List item markers are not always updated after changes in the DOM : [Attachment 53896] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jakub Wieczorek
<jwieczorek at webkit.org>'s request for review:
Bug 37060: List item markers are not always updated after changes in the DOM
https://bugs.webkit.org/show_bug.cgi?id=37060

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    RenderObject* child = this->nextInPreOrder(list);

No need for "this->" here.

> +	   if (newChild->isListItem())
> +	       toRenderListItem(newChild)->updateListMarkerNumbers();

After looking at this over and over again, I realized that we should probably
just put a virtual updateListMarkerNumbers function into RenderObject. Because
the code is just making a virtual function call to get the answer for whether
something is a list item, which is just a slower less elegant way of calling
virtual function in the first place.

> +    String markerText = markerTextForListItem(element);
> +    return g_strdup(markerText.utf8().data());

Seems to me we don't really need the local variable here.

>      friend class QWebFrame;
> +    friend class DumpRenderTreeSupportQt;
>      friend class QWebElementCollection;
>      friend class QWebHitTestResult;
>      friend class QWebHitTestResultPrivate;

Normally we keep these in alphabetical order.

> +    Element* element = listItem.m_element;
> +    return WebCore::markerTextForListItem(element);

Seems to me we don't really need the local variable here.