[Webkit-unassigned] [Bug 33668] WebCore::InsertListCommand::modifyRange infinite loop (100% CPU usage)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33668


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #63104|review?                     |review+
               Flag|                            |




--- Comment #23 from Darin Adler <darin at apple.com>  2010-08-24 12:47:49 PST ---
(From update of attachment 63104)
> +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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.