Bug 191483

<rdar://problem/45953305>

Created attachment 355791 [details] Patch

Comment on attachment 355791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355791&action=review > Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:-211 > - if (event.key === "a" && event.commandOrControlKey()) { Oops :P > Source/WebInspectorUI/UserInterface/Views/TreeElement.js:-293 > - element.treeElement.selectOnMouseDown(event); Is `selectOnMouseDown` called anywhere else? I know it's used by `WI.DOMTreeElement` and `WI.ShaderProgramTreeElement`. > Source/WebInspectorUI/UserInterface/Views/TreeElement.js:514 > let deselectedElement = treeOutline.selectedTreeElement; This variable is never used, so you could remove it. > Source/WebInspectorUI/UserInterface/Views/TreeElement.js:517 > + treeOutline.selectTreeElementInternal(this, suppressOnSelect, selectedByUser); This will have a slightly different effect than what was here before. It will no longer fire an event when the element is already selected, which was not the case previously (it would fire an event even it was already selected (`allowsRepeatSelection` had to have also been `true`). You should move this outside of the `if` (or just remove the `if` altogether): this.selected = true; treeOutline.selectTreeElementInternal(this, suppressOnSelect, selectedByUser); > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:59 > + this._treeElementIndexCache = new Map; Perhaps this would be a good opportunity to create an `IndexMap` (some sort of specialized `BiMap`)? > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:102 > + const extendSelection = false; This defaults to `false`, so you don't need to specify it in the `selectItem` call. > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:278 > + this.treeOutline._selectionController.didInsertItem(insertionIndex); Wow we do this all over the place... why is the `this.treeOutline` this needed? Would `this._selectionController` not suffice? Do we nest `WI.TreeOutline` somewhere? > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:576 > + if (this.selectedTreeElement) { This check isn't necessary, as you should've already bailed above if `!this.selectedTreeElement`. > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:631 > + handled = this._selectionController.handleKeyDown(event); Ah I see how you're using the return value. Nice! > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:921 > + ??? > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1015 > + let closestIndex = this._treeElementIndexCache.get(current); Is there a real need for this cache, other than in an effort to boost performance? It seems to me that since we clear it each time we remember/forget `WI.TreeElement`s, there's not much to be gained by having it. I'd just remove it unless it's necessary. > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1038 > + for (let treeElement = this.children[0]; treeElement; treeElement = treeElement.traverseNextTreeElement(skipUnrevealed, null, dontPopulate), ++current) { Style: wow this is ugly. I'd rework this as a `while`: const skipUnrevealed = false; const stayWithin = null; const dontPopulate = true; let current = 0; let treeElement = this.children[0]; while (treeElement) { if (current === index) return treeElement; treeElement = treeElement.traverseNextTreeElement(skipUnrevealed, stayWithin, dontPopulate); ++current; }

Comment on attachment 355791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355791&action=review >> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:59 >> + this._treeElementIndexCache = new Map; > > Perhaps this would be a good opportunity to create an `IndexMap` (some sort of specialized `BiMap`)? I considered this. Once the dust settles I'll take a look at cleaning up/optimizing TreeOutline a bit. >> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:921 >> + > > ??? Oops, this is old. >> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1015 >> + let closestIndex = this._treeElementIndexCache.get(current); > > Is there a real need for this cache, other than in an effort to boost performance? It seems to me that since we clear it each time we remember/forget `WI.TreeElement`s, there's not much to be gained by having it. I'd just remove it unless it's necessary. I'd like to leave it in for now. We can revisit/remove it later if needed.

Created attachment 355823 [details] Patch

Comment on attachment 355823 [details] 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} = {})

Created attachment 355830 [details] Patch

Created attachment 355831 [details] Patch

Comment on attachment 355831 [details] 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 😅

Comment on attachment 355831 [details] Patch Clearing flags on attachment: 355831 Committed r238599: <https://trac.webkit.org/changeset/238599>

All reviewed patches have been landed. Closing bug.

Next time please consider doing something like this behind the experimental flag. This broke debugger completely and introduced several bugs in the Elements tab.