[webkit-reviews] review granted: [Bug 86345] Deleting a content-editable element having a ShadowRoot causes a crash : [Attachment 145724] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 86345: Deleting a content-editable element having a ShadowRoot causes a
crash
https://bugs.webkit.org/show_bug.cgi?id=86345

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

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


> Source/WebCore/editing/VisibleSelection.cpp:457
> +	   // boundaries again. See http://wkb.ug/87463

Can we use the regular webkit bug URL instead? I don't see a benefit in using a
short URL in a comment like this.

> LayoutTests/editing/shadow/select-contenteditable-shadowhost.html:8
> +<p>This test confirms that selecting an element having Shadow DOM doesn't
cross editing boundaries errornously.</p>

Should we say something along the line of this test passes if WebKit didn't
crash?

> LayoutTests/editing/shadow/select-contenteditable-shadowhost.html:34
> +// Checks crash won't happen.
> +document.execCommand('delete');

You should use debug() to print out something to indicate we passed the test.

> LayoutTests/editing/shadow/select-contenteditable-shadowhost.html:46
> +// Check crash won't happen.
> +document.execCommand('delete');

Ditto.