[webkit-reviews] review denied: [Bug 217396] Web Inspector: Support three-column pane arrangement in Elements Tab : [Attachment 411158] Patch

Mon Oct 12 16:14:18 PDT 2020

Devin Rousso <drousso at apple.com> has denied Patrick Angle <pangle at apple.com>'s
request for review:
Bug 217396: Web Inspector: Support three-column pane arrangement in Elements
Tab
https://bugs.webkit.org/show_bug.cgi?id=217396

Attachment 411158: Patch

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

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

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

r-, but this is a good start!  In the future, a screenshot/video would've
really helped here :)

I started reviewing the actual code, but I think I have some questions about
the design so I'm gonna stop doing specific comments until those are
answered/resolved.

Having an explicitly "called out" primary/secondary sidebar seems perhaps
unnecessarily restrictive, and maybe a bit verbose.  When we'd chatted about
this offline, my thinking was more along the lines of "have the tab indicate
whether it accepts more than one sidebar (either as a `true`/`false` or as an
upper limit of the number of sidebars it can accommodate) and then have each
panel indicate whether it prefers being independent (similar to the `exclusive`
property of `WI.ScopeBarItem` or even `WI.NavigationItem.VisibilityPriority`). 
The reason I suggested this is because I think we should only be showing more
than one sidebar when we have the room for it (i.e. when Web Inspector is wide
enough), and on _really_ large screens we then could even have more than just
two sidebars (e.g. i would love to have [Styles | Computed | Changes] and [Node
| Layers] so that I can see CSS and Event Listeners side by side, but that
would require additional work to allow users to reorder sidebar panels across
multiple sidebars).  Also, I think assuming that the 0th sidebar is "the one
that should be separate" is something that could be inflexible to future change
and frankly kinda arbitrary.  At the very least it should be a property.

btw, it seems like with this patch that there would be no `WI.NavigationBar` at
the top of the Styles panel?  I think that would look really out of place
compared to the rest of Web Inspector.	Perhaps we could adjust the
pseudo-class toggles to always be visible when the Styles panel is in it's own
sidebar so that the spacing of things is at least preserved?

> Source/WebInspectorUI/UserInterface/Base/Main.js:284
> +    WI.detailsSidebar = new
WI.SidebarSet(document.getElementById("details-sidebar"),
WI.Sidebar.Sides.Trailing, null, WI.UIString("Details"));

this name should probably be changed

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:80
> +	   if (this._detailsSidebarPanelConstructors.length) {

NIT: why not nest this above?
```
    if (this._detailsSidebarPanelConstructors.length) {
	if (!this.allowsMultipleDetailSidebars) {
	    ...
	}
	...
    }

```

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:209
> +		   WI.detailsSidebar.secondaryPanel =
this.detailsSidebarPanels[0];

The naming of this almost seems kinda backwards to me ��

My first thought of this naming is that the primary/important panel would be
the Styles panel and then the secondary/ancillary panel(s) would be all the
other ones.

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:230
> +	   if (this._showDetailsSidebarItem)

We should always allow the details sidebar to be collapsed.  IMO if the user
has decided to enable the setting for multiple sidebar panels, we should always
respect that so long as Web Inspector is wide enough.  I think the navigation
item for expanding/collapsing the details sidebar should show/hide the all of
the details sidebars, not just whether or not we show the secondary panel.

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:35
> +	   this._allowResizeToCollapse = true;

NIT: maybe `allowResizingToCollapse`?

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:50
> +	       this._navigationBarAdditionalLeadingItems = new
WI.GroupNavigationItem;

Using `Items` here makes me think that this is an array, not a
`WI.GroupNavigationItem`.  Perhaps
`navigationBarAdditionalLeadingItemsGroup.navigationItems`?

Also, rather than expose the `WI.GroupNavigationItem` itself, maybe we should
only expose a `set leadingNavigationItems(navigationItems) {
this._navigationBarAdditionalLeadingItemsGroup.navigationItems =
navigationItems; }` for better encapsulation?  wdyt?

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:51
> +	       this._navigationBarAdditionalTrailingItems = new
WI.GroupNavigationItem;

is this actually needed?

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:96
> +	       // Index is +2 because of _navigationBarAdditionalLeadingItems
and a FlexibleSpaceNavigationItem
> +	      
this._navigationBar.insertNavigationItem(sidebarPanel.navigationItem, index +
2);

This seems like a hack.  Could you create a `WI.GroupNavigationItem` for the
panel navigation items in the `constructor` and add/remove items from that
instead?

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:165
> +    get allowResizeToCollapse()
> +    {
> +	   return this._allowResizeToCollapse;
> +    }
> +
> +    set allowResizeToCollapse(allow)
> +    {
> +	   this._allowResizeToCollapse = allow;
> +    }

Style: simple `get`/`set` can be collapsed onto a single line
```
    get allowResizingToCollapse() { return this._allowResizingToCollapse; }
    set allowResizingToCollapse(allowResizingToCollapse) {
this._allowResizingToCollapse = allowResizingToCollapse; }
```

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:235
> +    get navigationBarAdditionalLeadingItems()
> +    {
> +	   return this._navigationBarAdditionalLeadingItems;
> +    }

ditto (:157)
```
    get navigationBarAdditionalLeadingItems() { return
this._navigationBarAdditionalLeadingItems; }
```

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.css:39
> +    width: 0 !important;
> +    border: none !important;
> +    display: none;

Why do we need `width`/`border` if we're already `display: none;`?

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.css:40
> +}

NIT: missing trailing newline

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:45
> +	   this._primarySidebar = new WI.Sidebar(null, this._side, null, null,
label, true)

Style: missing `;`

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:46
> +	  
this._primarySidebar.addEventListener(WI.Sidebar.Event.CollapsedStateDidChange,
this._sidebarCollapsedStateDidChange, this, {sidebar: this._primarySidebar});

what is the `{sidebar: this._primarySidebar}` for?

NIT: I like to prefix all event listener functions with `_handle*` so that when
reading just that function in isolation it's understood that it's only supposed
to be called from an event listener.

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:49
> +	   this._secondarySidebar = new WI.Sidebar(null, this._side, null,
null, label, false);

It seems kinda weird to me to have both an array of sidebars and an explicitly
saved property.  I'd either drop the array concept or remove the explicit
property.

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:50
> +	  
this._secondarySidebar.addEventListener(WI.Sidebar.Event.CollapsedStateDidChang
e, this._sidebarCollapsedStateDidChange, this, {sidebar:
this._secondarySidebar});

what is the `{sidebar: this._secondarySidebar}` for?

ditto (:46)

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:62
> +	  
this._showSecondarySidebarNavigationItem.addEventListener(WI.ButtonNavigationIt
em.Event.Clicked, this._toggleSecondarySidebarVisible, this);

ditto (:46) e.g. `_handleShowSecondarySidebarButtonClicked`

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:73
> +    get sidebars()

ditto (Sidebar.js:157)

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:78
> +    get primarySidebar()

ditto (Sidebar.js:157)

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:83
> +    get secondarySidebar()

ditto (Sidebar.js:157)

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:88
> +    get sidebarPanels()

ditto (Sidebar.js:157)

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:125
> +    get side()

ditto (Sidebar.js:157)

I'd also move this up above.

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:133
> +	   if (!!sidebar instanceof WI.Sidebar)

I don't think this is gonna do what you want it to do.	We normally wrap the
`instanceof` inside parenthesis and then add a `!` before it (e.g. `!(sidebar
instanceof WI.Sidebar)`).

In this case, however, seeing as we already have a `console.assert` (and we
shouldn't ever be passing anything other than a `WI.Sidebar`), I think you can
just drop the `if` entirely :)

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:138
> +	   this._sidebars.push(sidebar);

NIT: we should also `console.assert(!this._sidebars.includes(sidebar), sidebar,
this);`

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:153
> +	   sidebar.removeEventListener(WI.Sidebar.Event.WidthDidChange,
this._sidebarWidthDidChange, this);

I'd move this below the `if` so that we only do this if we know that the
`sidebar` was inside `this._sidebars`.

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:158
> +	   if (!this._sidebars.includes(sidebar))
> +	       return;
> +
> +	   this._sidebars.remove(sidebar);

You can combine these and avoid the second iteration since
`Array.prototype.remove` (which is a utility method we define in
`Source/WebInspectorUI/UserInterface/Base/Utilities.js`) returns `true`/`false`
depending on whether an item was removed
```
    let removed = this._sidebars.remove(sidebar);
    console.assert(removed, sidebar, this);
```

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:177
> +	   if (this.sidebarPanels.indexOf(this.secondaryPanel) != -1 &&
this.sidebarPanels.indexOf(this.secondaryPanel) < index)

NIT: instead of calling `indexOf` twice, can you save it to a local variable?

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:189
> +	   for (let sidebar of this.sidebars) {

Style: no `{` `}` for single line control flow

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:210
> +	   for (let sidebar of this.sidebars) {

ditto (:189)

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:238
> +	   for (let sidebar of this.sidebars) {

ditto (:189)

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:288
> +	   } else if (typeof sidebarPanelOrIdentifierOrIndex === "number") {

ditto (:189)

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:383
> +	       this.collapsed = this.collapsed ||
this.primarySidebar.collapsed;

NIT: `||=`

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:344
> +	   if (this._ignoreSidebarEvents || !event.data)

When would `event.data` not be defined?