[webkit-reviews] review denied: [Bug 192116] Web Inspector: REGRESSION(r238602): Elements: deleting multiple DOM nodes doesn't select the nearest node after deletion : [Attachment 357019] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 192116: Web Inspector: REGRESSION(r238602): Elements: deleting multiple DOM
nodes doesn't select the nearest node after deletion
https://bugs.webkit.org/show_bug.cgi?id=192116

Attachment 357019: Patch

https://bugs.webkit.org/attachment.cgi?id=357019&action=review




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 357019
  --> https://bugs.webkit.org/attachment.cgi?id=357019
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357019&action=review

r-, as I'd expect slightly different functionality

Having a Delete > Nodes is a good idea :)

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:520
> +	   let selectedTreeElements =
this.selectedTreeElements.filter(hasNoSelectedAncestors);

This wouldn't have the effect I'd expect.  If I've selected a parent and child,
and press delete, I'd expect that they are no longer attached to the DOM tree
_and_ that their parent-child relationship no longer exists.  This would be
testable by having both nodes saved as $x values (e.g. `$1.parentNode !== $2 &&
$1.parentNode === null`).

Is the reason you're performing this check to ensure that the
`WI.SelectionController` doesn't try to do funky things?  You're already
`removeSelectedItems()` before removing each `WI.DOMTreeElement` individually,
so I don't think that's an issue there.