[webkit-reviews] review denied: [Bug 33668] WebCore::InsertListCommand::modifyRange infinite loop (100% CPU usage) : [Attachment 62374] reflects the cleanup done in 42841

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied 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 62374: reflects the cleanup done in 42841
https://bugs.webkit.org/attachment.cgi?id=62374&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
I didn't scrutinize the code too closely since the test output doesn't look
right to me.

> --- LayoutTests/ChangeLog	(revision 63943)
> +	   The behavior of WebKit when inserting ordered/unordered list on a
list with
> +	   an orphaned nested list at the end (5207369.html) is updated to
match that of MSIE instead of Firefox.

This doesn't match what I get in IE8. I get:
<P>One</P> 
<P>Two</P> 
<UL> 
<LI>Three</LI> 
<LI>Four</LI></UL>

> +++ LayoutTests/editing/execCommand/5207369-expected.txt	(working copy)
> -<ul><li>One</li><li>Two</li><li>Three</li><li>Four</li></ul>
>
+<ul><li>One</li><li>Two</li></ul><ol><ul><li>Three</li><li>Four</li></ul></ol>


This output doesn't look right to me. Why is the ul wrapped in an ol? FWIW, our
current behavior matches Opera. Firefox just inserts and LI at the top (clearly
wrong). IE removes the outer list entirely. In either case, this new output
doesn't match other browsers and seems worse than the old output. Seems like
the ideal behavior, from a user perspective, would be to output something like
<ul><li>One</li><li>Two</li><ul><li>Three</li><li>Four</li></ul></ul>. It would
be good to try this in Word, TextEdit and WordPad to see what they do in this
case.

> +++
LayoutTests/editing/execCommand/insert-list-orphaned-item-with-nested-lists.htm
l  (revision 0)
> +var e = document.getElementById('test')
> +var r = document.createRange();
> +var s = window.getSelection();

One letter variable names are frowned upon in webkit code. The one exception is
using "e" for the name of an event argument to a function might be OK since
it's so common.

> +r.setStart(e, 0);
> +r.setEnd(e, e.childNodes.length);
> +s.removeAllRanges();
> +s.addRange(r);
These four lines can just be s.selectAllChildren(e);

> +document.execCommand('InsertOrderedList', false, null);
> +
> +Markup.description("This tests crash when listifying nested lists with an
orphaned list child in between (see bug 33668).")
Technically, it's an infinite loop, not a crash, right?