[webkit-reviews] review granted: [Bug 181468] Web Inspector: TabBar redesign: improvements to tab layout and resize behavior : [Attachment 332408] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has granted Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 181468: Web Inspector: TabBar redesign: improvements to tab layout and
resize behavior
https://bugs.webkit.org/show_bug.cgi?id=181468

Attachment 332408: Patch

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




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

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

r=me, with some Style and NIT

> Source/WebInspectorUI/UserInterface/Views/GeneralTabBarItem.js:57
> +    get title()
> +    {
> +	   return super.title;
> +    }

Is this necessary?

> Source/WebInspectorUI/UserInterface/Views/GeneralTabBarItem.js:89
> +	   contextMenu.appendItem(WI.UIString("Close Tab"), () =>
this._parentTabBar.removeTabBarItem(this), this.isDefaultTab);

I think our style is to usually always have the {}  for inline functions on
ContextMenu items, as we never want to return any value.

    contextMenu.appendItem(WI.UIString("Close Tab"), () => {
	this._parentTabBar.removeTabBarItem(this);
    }, this.isDefaultTab);

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:410
> +	       item.element.classList.toggle("hidden", hidden);

One gotcha to keep an eye out for is that if `undefined` is passed as the
second argument to `classList.toggle` then it will just flip-flop the class
like a lightswitch.  You might want to add a `!!` in front of `hidden` so that
this can't happen:

    item.element.classList.toggle("hidden", !!hidden);

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:427
> +	   }

Style: missing semicolon

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:444
>  

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:609
> +		   contextMenu.appendItem(item.title, () =>
this.selectedTabBarItem = item);

This reads somewhat poorly.  I think it would be clearer to not make this one
line:

    for (let item of this._hiddenTabBarItems) {
	contextMenu.append(item.title, () => {
	    this.selectedTabBarItem = item;
	});
    }

Also, does this not need to call `needsLayout()` as it does in
`_handleTabPickerTabContextMenu`?