[webkit-reviews] review granted: [Bug 33668] WebCore::InsertListCommand::modifyRange infinite loop (100% CPU usage) : [Attachment 63104] fixed various problems.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 33668: WebCore::InsertListCommand::modifyRange infinite loop (100% CPU
usage)
https://bugs.webkit.org/show_bug.cgi?id=33668

Attachment 63104: fixed various problems.
https://bugs.webkit.org/attachment.cgi?id=63104&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +static Node* enclosingListChild(Node* node, Node* listNode)

I think it’s a little confusing that this function is just named
enclosingListChild even though it takes the two arguments. A clearer name might
help. You didn't add the function, so the name is not your fault.

> +		   // save and restore endOfSelection and startOfLastParagraph
when necessary
> +		   // since moveParagraph and movePragraphWithClones can remove
nodes.

The word "save" here should be capitalized.

> +		   // FIXME: This is very inefficient and expensive operation.

A better comment would explain why it's inefficient and why it's not easy to
fix or something like that. Just saying it’s inefficient is strange. Doesn’t
make clear what the tradeoff is.

> +		   int indexForEndOfSelection =
indexForVisiblePosition(endOfSelection);
>		   doApplyForSingleParagraph(forceCreateList, listTag,
currentSelection.get());
> +		   if (endOfSelection.isNull() || endOfSelection.isOrphan() ||
startOfLastParagraph.isNull() || startOfLastParagraph.isOrphan()) {
> +		       RefPtr<Range> lastSelectionRange =
TextIterator::rangeFromLocationAndLength(document()->documentElement(),
indexForEndOfSelection, 0, true);
> +		       endOfSelection = lastSelectionRange->startPosition();
> +		       startOfLastParagraph = startOfParagraph(endOfSelection);

> +		   }

This indexForEndOfSelection approach seems weak. It’s a sort of “I give up and
can’t figure out any other way to make it work” approach. Not that I have
anything better to suggest. As long as we have sufficient test coverage someone
can come back and tackle this later I guess.

> +	       // FIXME: moveParagraphWithClones (deleteSelection) does not
remove the old list properly
> +	       // See bug 33668 and the test
editing/execCommand/insert-list-orphaned-item-with-nested-lists.html

This comment is confusing. "Does not remove properly" doesn't tell you what it
does. It says what it doesn't do. It would be better if the comment was more
specific.

It seems impossible for one test to cover all the new code paths here. I'm
surprised you added only one test.