[webkit-reviews] review granted: [Bug 194299] REGRESSION: Elements tab: Uncaught Exception: No node with given id found : [Attachment 363703] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 194299: REGRESSION: Elements tab: Uncaught Exception: No node with given id
found
https://bugs.webkit.org/show_bug.cgi?id=194299

Attachment 363703: Patch

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




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

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

r=me

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:307
> +	   this._treeElementsToRemove.sort((a, b) => getLevel(b) -
getLevel(a));

Should we sort before calling `removeSelectedItems`?  It should make
performance slightly better since it organizes the array to be bottom-up based
on the DOM and we check bottom-up inside `canSelectTreeElement`, meaning that
the "lower" nodes are more likely to be earlier on in the array.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:330
> +	       return super.canSelectTreeElement(treeElement);

Rather than only using this path if we aren't removing, I think we should flip
this around.
```
    if (!super.canSelectTreeElement(treeElement))
	return false;

    let willRemoveAncestorOrSelf = false;
    if (this._treeElementsToRemove) {
	while (treeElement && !willRemoveAncestorOrSelf) {
	    willRemoveAncestorOrSelf =
this._treeElementsToRemove.includes(treeElement);
	    treeElement = treeElement.parent;
	}
    }
    return !willRemoveAncestorOrSelf;
```