[webkit-reviews] review granted: [Bug 192091] Web Inspector: REGRESSION(r238599): Multiple Selection: restoring selection when opening WebInspector puts the TreeElement into a permanent selected state : [Attachment 356347] 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 192091: Web Inspector: REGRESSION(r238599): Multiple Selection: restoring
selection when opening WebInspector puts the TreeElement into a permanent
selected state
https://bugs.webkit.org/show_bug.cgi?id=192091

Attachment 356347: Patch

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




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

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

r=me

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:400
>	   this._cachedNumberOfDescendents++;

Aside: we should assert that `_cachedNumberOfDescendants` is equal to the sum
of the length of every value in `_knownTreeElements`, or even just set it to be
that value (in the case that `treeElement` has already been remembered in the
past :

    console.assert(this._cachedNumberOfDescendents ===
Object.values(this._knownTreeElements).reduce((accumulator, current) =>
accumulator + current.length, 0));

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:416
>	   this._cachedNumberOfDescendents--;

Ditto (400)

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:994
>      _indexOfTreeElement(treeElement)

Aside: the uses of this in `selectionControllerNextSelectableIndex` and
`selectionControllerPreviousSelectableIndex` repeat a lot of work, as they both
iterate over the tree using `traverseNextTreeElement`.	Perhaps consider
combining/reworking them (followup)?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1010
> +	   return NaN;

We should add a "not-reached" assertion:

    console.assert(false, "Unable to get index for tree element", treeElement,
this);