[webkit-reviews] review denied: [Bug 235647] Web Inspector: [Flexbox] List flex containers in Layout sidebar : [Attachment 450063] Patch 1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Razvan Caliman
<rcaliman at apple.com>'s request for review:
Bug 235647: Web Inspector: [Flexbox] List flex containers in Layout sidebar
https://bugs.webkit.org/show_bug.cgi?id=235647

Attachment 450063: Patch 1.0

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




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

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

r-, please see my comment in
`Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js`

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.css:26
> +.details-section:is(.layout-css-flexbox:not(.collapsed),
.layout-css-grid:not(.collapsed)) > .content,

Would this also work?
```
.details-section:is(.layout-css-flexbox, .layout-css-grid):not(.collapsed) >
.content,
```

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:161
> +	       break;
> +	   case WI.DOMNode.LayoutContextType.Flex:

Style: I'd add a newline in between these lines.

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:164
> +	       break;
> +	   default:

ditto (:160)

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:167
> +	       this._flexNodeList.delete(domNode);
> +	   }

Style: missing a `break;`

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:194
> +	   this._flexNodeList = new
Set(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Flex)
);

Why are we suddenly calling these "List" even though they're actually `Set`?

> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.css:2
> + * Copyright (C) 2021 Apple Inc. All rights reserved.

2022

> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.css:31
> +.node-overlay-list-section .node-display-name {

Please put `>` in between selector components to be more specific (unless
there's an anonymous (i.e. no CSS class/id/etc.) in between).

ditto below

> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:59
> +	   console.assert(typeof fn === "function", fn);
> +	   this._showOverlay = fn;

This is a really weird pattern.  Normally we'd have something like
```
// Protected

showOverlay()
{
    // Implemented by subclasses.
}
```
which the subclass would then override (possibly also calling
`super.showOverlay()`).

This would also let you avoid having to do any `bind` and can help make the
internal JS object shape more consistent :)

> Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:137
> +	   // FIXME: Remove checks after implementing
<https://webkit.org/b/235649> Toggle Flexbox Page Overlay from Layout sidebar
> +	   if (this._showOverlay && this._hideOverlay) {

So what exactly is the "Flexbox Overlays" section gonna look like right now? 
Can you attach a screenshot?

I find it really odd that we're doing these patches in such incremental
non-functional steps instead of fully complete features.  It's more prone to
things being reworked as new details are revealed during further
implementation, resulting in greater code/git churn.  Why can't we do it all at
once?