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

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has asked  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 Ryosuke Niwa <rniwa at webkit.org>
(In reply to comment #20)
> (From update of attachment 63036 [details])
> > +		 ExceptionCode ec = 0;
> > +		 if (rangeStartIsInList && newList)
> > +		     currentSelection->setStart(newList->parentNode(),
newList->nodeIndex(), ec);
> > +		 if (rangeEndIsInList && newList)
> > +		     currentSelection->setEnd(newList->parentNode(),
newList->nodeIndex()+1, ec);
> 
> There should be spaces around the "+" operator here where we do "+ 1".

Modified the approach.	Used the positions inside the list instead.

> If you're going to ignore the exception code, there is no need to initialize
it to 0.

Fixed.

> Are all the code changes here covered by this single test?

When I modified the test to make sure rangeEndIsInList, there were other
problems with my code
to convert the entire list.  So I fixed them all and also this exposed a bug in
doApply where endOfSelection / startOfLastParagraph
can be orphaned / null.  Decided to save indexForVisiblePosition and restore it
later.
I know it's a dirty trick but because moveParagraphWithClones removes the
existing nodes, there is no other way to restore them.

I'm really worn away by all sorts of bugs in editing. For example, TextIterator
has a bug of not counting empty LI,
which causes index to shit by 1.  And that's the whole reason, I have that ugly
if statement to execute that line only if necessary.
But this bug in TextIterator will eventually expose if we added sufficiently
more test cases.

We need a better of keeping track of positions on nodes that are moved /
removed.  InsertListCommand / IndentOutdentCommand
are very fragile preciously because moveParagraph / moveParagraphWithClones
remove some nodes that these two
commands are using to iterate through paragraphs.