[webkit-reviews] review granted: [Bug 191483] Web Inspector: TreeOutline should re-use multiple-selection logic from Table : [Attachment 355831] 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 191483: Web Inspector: TreeOutline should re-use multiple-selection logic
from Table
https://bugs.webkit.org/show_bug.cgi?id=191483

Attachment 355831: Patch

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




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

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

r=me, well done!

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:-302
> -	   element.select();

Considering your changes to `WI.TreeOutline`, you could remove this entire
event listener.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:601
> +		   if (this.selectedTreeElement.expanded) {
> +		       nextSelectedElement =
this.selectedTreeElement.children[0];
> +		       while (nextSelectedElement &&
!nextSelectedElement.selectable)
> +			   nextSelectedElement =
nextSelectedElement.nextSibling;
> +		       handled = nextSelectedElement ? true : false;

This doesn't match Finder's functionality.  Are we sure we want to add this?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:873
> +    selectTreeElementInternal(treeElement, suppressNotification = false,
selectedByUser = false)

I'd rather you use an optional object instead of defaulted parameters:

    selectTreeElementInternal(treeElement, {suppressNotification,
selectedByUser} = {})

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:875
> +	   if (this._processingSelectionControllerSelectionDidChange)

All of this early-return interleaving is a bit crazy.  We definitely need some
tests for all of these different cases 😅