[webkit-reviews] review granted: [Bug 193089] Web Inspector: Elements tab should toggle visibility for all selected nodes : [Attachment 360404] 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 193089: Web Inspector: Elements tab should toggle visibility for all
selected nodes
https://bugs.webkit.org/show_bug.cgi?id=193089

Attachment 360404: Patch

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




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

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

r=me, much nicer than the last version :)

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:295
> +    toggleElementVisibility(forceHidden)

If only we could share more of this code with `DOMNode.prototype.toggleClass`
:(

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:727
> +	   let selectedTreeElements = this.treeOutline ?
this.treeOutline.selectedTreeElements : [];

I'd rather you inline this in the `if` than construct a temporary array.

    if (this.selected && this.treeOutline &&
this.treeOutline.selectedTreeElements.length > 1)

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:201
> +	   if (!selectedTreeElements.length)

NIT: I find these types of checks unnecessary, as we'd effectively return
inside the loop anyways (iterating over an array of no length has the same
effect as returning early, albeit the latter is "maybe" quicker).

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:554
> +	   let allHidden = this.selectedTreeElements.every((treeElement) =>
treeElement.isNodeHidden);

This should either match the name of the parameter (e.g. `forceHidden`), or be
inlined.  Since the function is defined on this class, it's much easier for a
developer to be able to see this quickly than if it were somewhere else.