[webkit-reviews] review denied: [Bug 192388] Web Inspector: Table selection becomes corrupted when deleting selected cookies : [Attachment 356772] 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 192388: Web Inspector: Table selection becomes corrupted when deleting
selected cookies
https://bugs.webkit.org/show_bug.cgi?id=192388

Attachment 356772: Patch

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




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

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

r-, as I think there is another issue with this.

If [1, 2, 3] are selected, and both [1, 2] are removed simultaneously, I'd
expect that [3] becomes [1].  This can happen if we had a DOM tree like this:

    <0>
	<1>
	    <2></2>
	</1>
    </0>
    <3></3>

If <0> gets removed from the DOM tree via JavaScript, <1> and <2> would get
removed from the DOM tree without the frontend/user actually explicitly calling
delete.

I'm pretty sure that right now, because we call `_forgetTreeElement` on the
parent and _then_ the child, we'd hit the assertion that we couldn't get an
index for <1> and <2> since they'd already have been removed from the
`WI.TreeOutline`.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:409
> +	   // Update the SelectionController before attaching the TreeElement.
> +	   // Attaching the TreeElement can cause it to become selected, and
> +	   // added to the SelectionController.

NIT: is this comment strictly necessary, or is it mainly for the benefit of
future programming, like as a "gotcha"?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:410
> +	   let index = this._indexOfTreeElement(element) || 0;

Should we even be calling `didInsertItem` if we're unable to get an index?  You
already assert inside `_indexOfTreeElement`, so this seems a bit counter to
that expectation.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:422
> +	       this._selectionController.didRemoveItems(new
WI.IndexSet([index]));

Ditto (>410).