[webkit-reviews] review granted: [Bug 192116] Web Inspector: REGRESSION(r238602): Elements: deleting multiple DOM nodes doesn't select the nearest node after deletion : [Attachment 357248] 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 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 357248: Patch

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




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

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

r=me, nice work :)

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:287
> +	       if (!levelMap.has(element)) {

This performs two lookups, which can be avoided.

    function getLevel(treeElement) {
	let level = levelMap.get(treeElement);
	if (isNaN(level)) {
	    level = 0;
	    let current = treeElement;
	    while (current = current.parent)
		++level;
	    levelMap.set(treeElement, level);
	}
	return level;
    }

Style: you're reusing `level` as both a function name and variable name.
Style: since we're expecting `WI.TreeElement` and not DOM nodes, the parameter
should be `treeElement` instead of `element`.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:302
> +	   // same DOMNode can both be selected.

NIT: I'd either include the `WI` or separate it into two names, for clarity as
to whether you're referring to our model object or the builtin Node (e.g.
`WI.DOMNode` vs DOM node).