[webkit-reviews] review denied: [Bug 43166] InsertOrderedList does not switch the list type properly when it has an inner list. : [Attachment 62913] fixes the bug

[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 62913: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=62913&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good. A few comments.

> +    if (previousList && previousList->isHTMLElement() &&
canMergeLists(previousList, listElement))

> +    if (nextList && nextList->isHTMLElement() && canMergeLists(listElement,
nextList)) {

If we need an isHTMLElement check here, then why does canMergeLists take an
Element*?

If canMergeLists takes an Element*, then should it do the isHTMLElement check
itself?

> +    const QualifiedName listTag = (m_type == OrderedList) ? olTag : ulTag;

This should be const QualifiedName& rather than const QualifiedName, to
generate slightly more efficient code and avoid a ref/deref of the tag.

> +    RefPtr<Range> currentSelection = endingSelection().firstRange();
> +    doApplyForSingleParagraph(false, listTag, currentSelection.get());

You could do this without a local variable if you think that would be more
readable.

    doApplyForSingleParagraph(false, listTag,
endingSelection().firstRange().get());

> +    bool selectionHasListOfType(const VisiblePosition& start, const
VisiblePosition& end, const QualifiedName&);

Too bad we have to pass two VisiblePosition. It would be nice if "selection"
was a single argument.

> +    HTMLElement* mergeWithNeighboringLists(HTMLElement*);

Using raw pointers in cases like this can be unsafe if there are DOM mutation
events involved. You might need the result to be PassRefPtr in case the list is
destroyed during the merging process.

Do the new tests really cover all the branches in the code? I see lots of
branches in the code and only a couple test cases. I don't think there is
enough test coverage here.

review- so you can at least fix the const QualifiedName& thing.