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

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
> 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>

It's only visually identical in the absence of CSS, e.g., <style>ul, ol
{border: 1px solid blue}</style> would render an extra border.

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

The old results for this test are clearly wrong and the new results for this
test are wrong. But I worry that the new results are more wrong. I don't think
you need to make this test match IE in this patch, but is there a way to avoid
the infinite loop that doesn't change the results for this test?