[webkit-reviews] review requested: [Bug 33668] WebCore::InsertListCommand::modifyRange infinite loop (100% CPU usage) : [Attachment 62487] fixed per ojan's comments

[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 62487: fixed per ojan's comments
https://bugs.webkit.org/attachment.cgi?id=62487&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
(In reply to comment #13)
> 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>

I think you did InsertOrderedList as supposed to InsertUnorderedList.  When I
do InsertUnorderedList on MSIE, I get:
<ul>
<li>one</li>
<li>two</li>
<ul>
<li>three</li>
<li>four</li>
</ul>
</ul>

But this is visually identical to
<ul>
<li>one</li>
<li>two</li>
</ul>
<ol>
<ul>
<li>three</li>
<li>four</li>
</ul>
</ol>

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

Again, I think you did InsertOrderedList.

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

Right.	That's what I'm trying to accomplish here but with our current
implementation, we can't tell if the outer list is contained within the
selection because we modify the selection.  So the best we can do is to leave
the outer OL intact.  I could add a logic to merge outer lists when the only
content of the list is another list.  That will produce the above HTML as
desired.  Ideally, when the entire list is selected, we can convert the type of
the list element without having to work on each paragraph separately and then
merge.

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

Good point.

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

Right.