[webkit-reviews] review granted: [Bug 191483] Web Inspector: TreeOutline should re-use multiple-selection logic from Table : [Attachment 355823] 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 355823: Patch

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




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

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

r=me, awesome work!

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:192
> +	   return true;

Add "// Overridden by subclasses if needed."

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:278
> +	   if (this.allowsMultipleSelection) {
> +	       let insertionIndex =
this._indexOfTreeElement(child.previousSibling) || 0;
> +	      
this.treeOutline._selectionController.didInsertItem(insertionIndex);
> +	   }

I think this should be always called.  Whenever we add a new item, even if
we're not allowing multiple selection, we should update the index of any
selected item(s) anywhere in the tree.	As an example, if I've selected item 3,
and I insert at item 1, my selection should be updated to item 4.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:602
> +		   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:835
> +	   while (treeElement =
treeElement.traverseNextTreeElement(skipUnrevealed, null, dontPopulate)) {

NIT: use `const stayWithin = null;` for this argument instead.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:852
> +	   while (treeElement =
treeElement.traversePreviousTreeElement(skipUnrevealed, null, dontPopulate)) {

Ditto (835)

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

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

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