[webkit-reviews] review denied: [Bug 67764] Crash in WebCore::CompositeEditCommand::insertNodeAt : [Attachment 116615] Proposed patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Parag Radke <nrqv63 at motorola.com>'s
request for review:
Bug 67764: Crash in WebCore::CompositeEditCommand::insertNodeAt
https://bugs.webkit.org/show_bug.cgi?id=67764

Attachment 116615: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=116615&action=review

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


> Source/WebCore/ChangeLog:8
> +	   Added a check for root editability of node's parent.

You should explain why this fixes the crash and why your fix is correct.

> Source/WebCore/editing/CompositeEditCommand.cpp:232
> +    if (parent && !parent->rootEditableElement())

I think what you're trying to check is that parent->isContentEditable().

> LayoutTests/ChangeLog:8
> +	   Added a test case for the crash-67764.

crash-67764 doesn't tell me anything about the kind of crash you're fixing.
Please explain rather than giving the bug number.
Also, the bug number is redundant because it's already given at line 4.

> LayoutTests/editing/deleting/delete-block-merge-contents-025.html:6
> +var sel = window.getSelection();
> +sel.setPosition(div, 2000000000);
> +document.execCommand("Delete");

This should be converted to a dumpAsText test. Just do:
if (window.layoutTestController)
    layoutTestController.dumpAsText();