[webkit-reviews] review granted: [Bug 192119] Web Inspector: Elements: $0 is shown for all selected elements : [Attachment 356401] 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 192119: Web Inspector: Elements: $0 is shown for all selected elements
https://bugs.webkit.org/show_bug.cgi?id=192119

Attachment 356401: Patch

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




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

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

r=me

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:465
> +	   if (domNode && selectedByUser)
> +	       WI.domManager.highlightDOMNode(domNode.id);

If `domNode` is falsy, we should hide the highlight by calling
`WI.domManager.hideDOMNodeHighlight();`.

Side note: I just noticed, but I think this is broken:
<https://webkit.org/b/192354>

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:468
> +	   this._domTreeOutline.suppressRevealAndSelect = false;
> +	   this._domTreeOutline.updateSelectionArea();

Not sure that it matters, but this is reversed from what was there before.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:-628
> -    onselect(treeElement, selectedByUser)

My only concern with something like this is the order of operations, in that
`onselect` was guaranteed to happen before event dispatch, but considering that
this doesn't change the selection, I think it's fine.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:81
> +.tree-outline.dom li.last-selected > span::after {

The rule after this (`.tree-outline.dom:focus li.selected > span::after`)
should probably also be changed to `.last-selected`.

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:501
> -    select(omitFocus, selectedByUser, suppressOnSelect, suppressOnDeselect)
> +    select(omitFocus, selectedByUser, suppressNotification)

I really would prefer that you change this to an optional object in this patch.
 You're already changing almost all of the callers due to the argument removal,
so it's not crazy to change them to this pattern at the same time.  Using a
list of flags is really not very readable or future-proof, as evident in much
of this patch.	I had to keep going back/forth multiple times to remember what
each argument meant in all of the callers of `select`, `revealAndSelect`, and
`deselect` (even worse for callers that just pass `true` or `false`).  Using an
optional object makes it clear at the call-site what each argument means, and
makes it so that the callers only have to provide values for things they want
"on".