[webkit-reviews] review requested: [Bug 43166] InsertOrderedList does not switch the list type properly when it has an inner list. : [Attachment 62975] fixed per darin's comments

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has asked  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 62975: fixed per darin's comments
https://bugs.webkit.org/attachment.cgi?id=62975&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
(In reply to comment #2)
> (From update of attachment 62913 [details])
> Looks good. A few comments.

Thanks for the 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*?

Good point. So the reason canMergeLists takes two Element* is because
previousSiblingElement/nextSiblingElement returns Element and having to check
that they are indeed HTMLElement is usually redundant since we check the tag
name in canMergeLists, and one of the list is created by us or returned by
enclosingList.	Added a check to make sure they're indeed HTMLElement in
canMergeLists.

> 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.

Ah! I didn't catch that. Fixed.

> > +	 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());

Mn.. but doesn't that lead to a memory leak since firstRange() returns a
PassRefPtr?  Because I'm passing the same range object to multiple
doApplyForSingleParagraph in the case of range selection, I don't really want
doApplyForSingleParagraph to take PassRefPtr.

> > +	 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.

Changed to take VisibleSelection.

> > +	 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.

Fixed.

> 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.

Sorry, I forgot to fix the test.  I'm adding two tests here because the list
child added by fixOrphanedListChild is supposed to be merged with surrounding
lists and this was not tested.