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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 19 12:08:36 PDT 2020 

Devin Rousso <drousso at apple.com> has granted 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 411640: Patch

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




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

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

r=me, awesome work!

I have a few more style fixes.	As a general piece of advice, consider
installing <https://eslint.org> and hooking it up to your editor, as that
should (hopefully) catch many issues before review (note that we don't exactly
follow eslint, such as including extra parenthesis/escapes for clarity).  You
may have to slightly change the config to get proper linting for new language
features tho (i think 2018 -> 2020 for optional chaining or logical
assignment).

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.css:33
> +    flex-direction: row-reverse;

clever ��

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:52
> +    get primarySidebar() {

Style: `{` on separate line

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:56
> +    get allowMultipleSidebars() {

ditto (:52)

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:70
> +    get multipleSidebarsVisible() {

ditto (:52)

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:116
> +    get selectedSidebarPanel() {

ditto (:52)

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:120
> +    set selectedSidebarPanel(sidebarPanelOrIdentifierOrIndex) {

ditto (:52)

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:124
> +    get collapsable() {

ditto (:52)

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:144
> +	   return this.element.offsetWidth; 

unnecessary trailing space

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:258
> +	       for (var i = index - 1; i >= 0; i--) {

`let`

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:273
> +	   for (let sidebar of this._sidebars)

Style: we only omit the `{` `}` when the body of the control flow is a single
line

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:277
> +	   for (let sidebar of this._sidebars)
> +	       if (sidebar.sidebarPanels.includes(sidebarPanel))
> +		   return sidebar;
> +
> +	   return null;

I think you can simplify this
```
    return this._sidebars.find((sidebar) =>
sidebar.sidebarPanels.includes(sidebarPanel)) || null;
```

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsSidebarPanel.js:35
> +    get allowExclusivePresentation() {

ditto (MultiSidebar.js:52)

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:398
> +	   experimentalSettingsView.addSetting(WI.UIString("Elements Tab:",
"Elements Tab: @ Experimental Settings", "Category label for experimental
settings pertaining to the Elements tab"),
WI.settings.experimentalEnableExclusiveDetailsPanels, WI.UIString("Show
independent Styles sidebar"));

NIT: We should have the name of the `WI.ExperimentalSetting` match the name of
the toggle.  How about `experimentalEnableIndepdendentStylesPanel`?  Generally
speaking it's a good idea to have the names of things in code match the name of
things in the UI (I'd also consider changing `WI.SidebarPanel.prototype.get
exclusive` to `get independent` as well, but that's up to you).

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:90
> +    get selectedSidebarPanel() {

ditto (MultiSidebar.js:52)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:112
> +    get collapsed() {

ditto (MultiSidebar.js:52)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:139
> +    get collapsable() {

ditto (MultiSidebar.js:52)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:143
> +    set collapsable(allow) {

ditto (MultiSidebar.js:52)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:168
> +    shouldInsertSidebarPanel(sidebarPanel, index) {

ditto (MultiSidebar.js:52)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:169
> +	   /* Implemented by subclasses if needed. */

`//`

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:173
> +    didInsertSidebarPanel(sidebarPanel, index) {

ditto (MultiSidebar.js:52)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:174
> +	   /* Implemented by subclasses if needed. */

ditto (:169)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:177
> +    didRemoveSidebarPanel(sidebarPanel) {

ditto (MultiSidebar.js:52)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:178
> +	   /* Implemented by subclasses if needed. */

ditto (:169)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:181
> +    willSetSelectedSidebarPanel(sidebarPanel) {

ditto (MultiSidebar.js:52)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:182
> +	   /* Implemented by subclasses if needed. */

ditto (:169)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:185
> +    didSetSelectedSidebarPanel(sidebarPanel) {

ditto (MultiSidebar.js:52)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:186
> +	   /* Implemented by subclasses if needed. */

ditto (:169)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:189
> +    didSetCollapsed(flag) {

ditto (MultiSidebar.js:52)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:190
> +	   /* Implemented by subclasses if needed. */

ditto (:169)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:197
> +	   var sidebarPanel = null;

`let`

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:205
> +	       for (var i = 0; i < this._sidebarPanels.length; ++i) {

ditto (:197)

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:210
> +	       for (var i = 0; i < this._sidebarPanels.length; ++i) {
> +		   if (this._sidebarPanels[i].identifier ===
sidebarPanelOrIdentifierOrIndex) {
> +		       sidebarPanel = this._sidebarPanels[i];
> +		       break;
> +		   }
> +	       }

I think you can simplify this
```
    sidebarPanel = this._sidebarPanels.find((existingSidebarPanel) =>
existingSidebarPanel.identifier === sidebarPanelOrIdentifierOrIndex) || null;
```

> Source/WebInspectorUI/UserInterface/Views/SidebarPanel.js:96
> +    get exclusive() { return this._exclusive; }

Style: We only inline `get`-`set` pairs when both are inlined.	In this case,
since the `set` is multiple lines, please also make the `get` multiple lines.

> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:63
> +    get width() {

ditto (MultiSidebar.js:52)

> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:92
> +    {	

unnecessary trailing whitespace

> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:141
> +	   var newWidth = positionDelta + this._widthBeforeResize;

`let`

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:319
> +	       assert(false, "panel selection not allowed for navigation
sidebar")

`console.assert`

Also, I'd personally suggest changing this back to the way it was, since we can
assume that `event.target` will never be `this._navigationSidebar` since we
only add this event listener to `this._detailsSidebar` :)

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:323
> +	       if (tabContentView.managesDetailsSidebarPanels)
> +		   return;

we already did this check above so it doesn't need to be repeated

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:327
> +

Style: unnecessary newline

More information about the webkit-reviews mailing list