[webkit-reviews] review denied: [Bug 43166] InsertOrderedList does not switch the list type properly when it has an inner list. : [Attachment 63000] made the call to endingSelection().firstRange().get() inline

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 63000: made the call to endingSelection().firstRange().get() inline
https://bugs.webkit.org/attachment.cgi?id=63000&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +PassRefPtr<HTMLElement>
InsertListCommand::mergeWithNeighboringLists(HTMLElement* listElement)
> +{
> +    Element* previousList = listElement->previousElementSibling();
> +    Element* nextList = listElement->nextElementSibling();
> +    if (canMergeLists(previousList, listElement))
> +	   mergeIdenticalElements(previousList, listElement);
> +    if (canMergeLists(listElement, nextList)) {
> +	   mergeIdenticalElements(listElement, nextList);
> +	   return static_cast<HTMLElement*>(nextList);
> +    }
> +    return listElement;
> +}

Thanks for changing the function result into a PassRefPtr.

If DOM mutation events can run while we make changes, then this code is unsafe.
After either of the two calls to mergeIdenticalElements returns, listElement or
nextList might be pointing to a deleted object. Thus, they should both be
RefPtr or PassRefPtr.

Calling nextElementSibling after handling previousList would be another way of
avoiding using a possibly-bad nextList pointer, but it wouldn't help you safely
return nextList after calling mergeIdenticalElements, so I think you need a
RefPtr.

The patch otherwise looks great.

review- because I'd like you to use RefPtr more to avoid potential
use-after-free as explained above