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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
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

Attachment 411158: Patch


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

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

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.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 =

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

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

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

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 +

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

> 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._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

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:50
> +	  
e, this._sidebarCollapsedStateDidChange, this, {sidebar:

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

ditto (:46)

> Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:62
> +	  
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,

> 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 ||

NIT: `||=`

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

When would `event.data` not be defined?

More information about the webkit-reviews mailing list