[webkit-reviews] review granted: [Bug 89918] [Shadow] Triggers assertion in VisibleSelection::adjustSelectionToAvoidCrossingBoundaries() : [Attachment 149769] 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 89918: [Shadow] Triggers assertion in
VisibleSelection::adjustSelectionToAvoidCrossingBoundaries()
https://bugs.webkit.org/show_bug.cgi?id=89918

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

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


> LayoutTests/ChangeLog:12
> +	   However, we don't have any test case of selection from Shadow DOM to
elements outside of shadow host.
> +	   So let's add a testcase here.

I don't think you need to mention this. It's pretty obvious that the reason
we're adding a regression test is because we hadn't had one already.

> LayoutTests/editing/shadow/breaking-editing-boundaries-2.html:6
> +<script src="../../fast/dom/resources/event-sender-util.js"></script>
> +<script src="../../fast/js/resources/js-test-pre.js"></script>

I don't think you need a event-sender-util.js here. You're not using any
functions in there.

Also, I don't see a point in making this a script test. You're not using any
features in there.

> LayoutTests/editing/shadow/breaking-editing-boundaries-2.html:14
> +    <div id="host">host</div> 
> +    <div id="dst">after host</div>

Please spell out destination.

> LayoutTests/editing/shadow/breaking-editing-boundaries-2.html:29
> +var src = shadowRoot.getElementById('src');
> +var dst = document.getElementById('dst');

Ditto about spelling out source and destination.

> LayoutTests/editing/shadow/breaking-editing-boundaries-2.html:37
> +    container.innerHTML = "PASS";

This is sufficient without any of js-test-*.js files.