[webkit-reviews] review denied: [Bug 53409] InsertUnorderedList over a non-editable region and multiple lines enters an infinite loop : [Attachment 82512] Patch

Thu Mar 3 00:59:10 PST 2011

Ryosuke Niwa <rniwa at webkit.org> has denied Levi Weintraub
<leviw at chromium.org>'s request for review:
Bug 53409: InsertUnorderedList over a non-editable region and multiple lines
enters an infinite loop
https://bugs.webkit.org/show_bug.cgi?id=53409

Attachment 82512: Patch
https://bugs.webkit.org/attachment.cgi?id=82512&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82512&action=review

The change looks sane but I'd like to see some brush ups.

> Source/WebCore/editing/InsertListCommand.cpp:181
> +		   } while (nextStart.isNotNull() && nextStart !=
startOfLastParagraph && nextStart == nextEnd
> +			    && (endOfParagraph(nextEnd,
CanCrossEditingBoundary) != startOfParagraph(nextStart,
CanCrossEditingBoundary)

We should probably put all these condition into some inline function with a
descriptive name.

> Source/WebCore/editing/InsertListCommand.cpp:182
> +			    || nextStart.deepEquivalent().anchorType() ==
Position::PositionIsOffsetInAnchor));

I don't understand why we need this condition.

> Source/WebCore/editing/InsertListCommand.cpp:394
> -    moveParagraph(start, end, positionBeforeNode(placeholder.get()), true);
> +    bool preserveSelection = true;
> +    moveParagraph(start, end, positionBeforeNode(placeholder.get()),
preserveSelection);

left over?

> Source/WebCore/editing/visible_units.cpp:762
> +	       while (n && n->isContentEditable() !=
startNode->isContentEditable())
> +		   n = n->traversePreviousNodePostOrder(startBlock);

This can move from one editable region to another that don't share the highest
editable root.	e.g. WebKit to hello in <span contentditable>hello</span> world
<span contenteditale>WebKit</span>.  Is that intended behavior?  It seems odd
that "skip over" means crossing arbitrary editing boundaries.

> LayoutTests/editing/execCommand/insert-list-with-noneditable-content.html:1
> +<div contenteditable="true" id="test" style="padding: 1em;">

You should put <!DOCTYPE html> here.

> LayoutTests/editing/execCommand/insert-list-with-noneditable-content.html:3
> +  Editable paragraph containing a <span contenteditable="false"
style="background-color: LightGray;">non-editable</span> span.<br />
> +  Another editable paragraph.

xml style br?  Is that required?

> LayoutTests/editing/execCommand/insert-list-with-noneditable-content.html:17
> +  function insertList() {
> +	 document.execCommand('insertunorderedlist', false, '');
> +  }
> +
> +  var s = window.getSelection();
> +  var div = document.getElementById("test");
> +  s.setPosition(div.childNodes[0], 10);
> +  s.modify("extend", "forward", "line");
> +  
> +  if (window.layoutTestController)
> +	 insertList();

Nit: We don't normally indent script like this.  Please outdent them.

I don't think you have enough descriptions on this test.  When I just look at
this test, I don't know what is the correct behavior.  You should probably
explain that.  And I highly doubt that this test requires pixel results.  You
should probably call dumpAsText here so that we can share the expected results
across platforms.