[webkit-reviews] review denied: [Bug 57079] DeleteSelectionCommand::removeNode tries to insert block placeholder in non-editable table cell positions : [Attachment 86891] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Benjamin (Ben) Kalman
<kalman at chromium.org>'s request for review:
Bug 57079: DeleteSelectionCommand::removeNode tries to insert block placeholder
in non-editable table cell positions
https://bugs.webkit.org/show_bug.cgi?id=57079

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

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

>
LayoutTests/editing/execCommand/delete-table-with-empty-contents-expected.txt:1

> +Tests that tables with inevitably empty content are deleted correctly.

I'm not sure if "inevitably empty content" is self-explanatory.  I wouldn't
mind it be a little more verbose.

> LayoutTests/editing/execCommand/delete-table-with-empty-contents.html:2
> +<div contenteditable tests='empty'>

I'm not sure if it's really desirable to have custom "tests" attribute.  Is it
allowed in HTML5 spec?	I mean what if in the future we decided to add tests
attribute for whatever purpose in HTMLx.  Wouldn't this collide with that? 
Ideally, we wouldn't need to worry about this test when that happens.

> LayoutTests/editing/execCommand/delete-table-with-empty-contents.html:73
> +	 'FAIL (' + testDescription + '), should be empty but was ' +
container.innerText;

Not necessarily "empty", right?  How about 'should contain exactly one LF but
was' or 'should be "\n" but was'

> Source/WebCore/editing/DeleteSelectionCommand.cpp:344
> +    Node* next = node;
> +    while (next) {
> +	   if (next->isContentEditable())
> +	       return firstPositionInNode(next);
> +	   next = next->traverseNextNode(node);
> +    }

First off, it's wrong to always return firstPositionInNode because next could
be img element with width & height 0px. r- because of this.

I would make it a for loop as in:
for (Node* next = node; next && !next->isContentEditable(); next =
next->traverseNextNode(node)) {}
return firstPositionInOrBeforeNode(next);

> Source/WebCore/editing/DeleteSelectionCommand.cpp:389
> +	       Position firstEditablePosition =
firstEditablePositionInNode(node.get());

Presumably firstEditablePosition is visually coinciding with
firstPositionInOrBeforeNode(node).  Can we assert that by e.g. comparing that
the canonicalized visible positions created by those two positions are
identical?