Web Inspector: Display all CSS grids on page in Layout panel

[Nikita Vasilyev]

Add "Grid" section to the Layout panel (experimental).

A list my look like this:
  div.wrapper
  div.box.a
  div.box.b

Add a list of all elements that have `display: grid` or `display: inline-grid`. To prototype this, we can get all elements with querySelectorAll and filter the ones we need by looking at the computed styles data.

[Radar WebKit Bug Importer]

<rdar://problem/73764515>

[Nikita Vasilyev]

Created attachment 418797 [details]
WIP

This is NOT for review. I'm handing off to Razvan where I left off so he could continue working on it before I wake up on Monday.

[Nikita Vasilyev]

Comment on attachment 418797 [details]
WIP

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

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:35
> +            console.info("gridNodes", gridNodes);

On https://gridbyexample.com/examples/code/example1.html this should log one one node: <div class="wrapper">.

[Razvan Caliman]

Created attachment 418902 [details]
WIP v1.1

Not for review. 

Work in progress on listing grid container nodes in Layout panel and event handlers to toggle the CSS Grid overlay

[Nikita Vasilyev]

Created attachment 418904 [details]
[Image] With patch applied

[BJ Burg]

Comment on attachment 418902 [details]
WIP v1.1

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

Added some comments.

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:59
> +        let requestDocumentPromise = new Promise(function(resolve, reject) {

This will not be necessary after my patch lands, as it makes requestDocument return a Promise in the absence of a callback argument.

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:66
> +            return Promise.resolve(null);

I think this should reject, no?

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:69
> +        let promises = ids.map(function(nodeId) {

Absent other reasons, we tend to prefer arrow functions simply because it's shorter.

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:102
> +      let linkList = this.element.querySelector(".node-link-list");

You should just store linkList as a member.

NIT: convention is to name variables holding DOM elements with a *Element suffix. e.g., this._linkListElement

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:108
> +        linkList.setAttribute("class", "node-link-list");

Nit: we usually set this up via .classList or .className

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:113
> +      for (const domNode of gridNodes) {

Nit: let domNode

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:116
> +        const checkbox = document.createElement("input");

In this codebase, we only use const for scalar constants.

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:119
> +        checkbox.checked = false; // TODO: sync with state of overlay from a model.

Suggestion: add WI.GridOverlayManager to manage the list of WI.GridOverlay objects and their properties. Notably, the backend requires one DOMAgent.showGridOverlay call per overlay to update drawing settings, so the manager would need to deal with that. Later on, it will also deal with saving and restoring settings / colors.

[Nikita Vasilyev]

Created attachment 418961 [details]
WIP

Rebaselined.
Added Views/GridDetailsSectionRow.css.
Added color swatches. Currently shows the same color for each grid (“magenta”).

BJ’s comments are largely unaddressed.

[Razvan Caliman]

Created attachment 419020 [details]
WIP v1.2

Added OverlayManager controller and hooked up to logic to toggle grid overlay

[Razvan Caliman]

Created attachment 419051 [details]
Patch

[Razvan Caliman]

Created attachment 419052 [details]
[Image] With patch applied

[Razvan Caliman]

Comment on attachment 418902 [details]
WIP v1.1

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

>> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:59
>> +        let requestDocumentPromise = new Promise(function(resolve, reject) {
> 
> This will not be necessary after my patch lands, as it makes requestDocument return a Promise in the absence of a callback argument.

Done!

>> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:66
>> +            return Promise.resolve(null);
> 
> I think this should reject, no?

Done!

>> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:69
>> +        let promises = ids.map(function(nodeId) {
> 
> Absent other reasons, we tend to prefer arrow functions simply because it's shorter.

Done!

>> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:102
>> +      let linkList = this.element.querySelector(".node-link-list");
> 
> You should just store linkList as a member.
> 
> NIT: convention is to name variables holding DOM elements with a *Element suffix. e.g., this._linkListElement

Thanks for the tip! Done!

>> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:108
>> +        linkList.setAttribute("class", "node-link-list");
> 
> Nit: we usually set this up via .classList or .className

Done!

>> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:119
>> +        checkbox.checked = false; // TODO: sync with state of overlay from a model.
> 
> Suggestion: add WI.GridOverlayManager to manage the list of WI.GridOverlay objects and their properties. Notably, the backend requires one DOMAgent.showGridOverlay call per overlay to update drawing settings, so the manager would need to deal with that. Later on, it will also deal with saving and restoring settings / colors.

I added a generic OverlayManager to handle Grids and upcoming overlays. It implements generic show/hide events and dispatches generic "overlay-shown"/"overlay-hidden" events for consumers to handle (Layout panel, Elements panel).

[Devin Rousso]

Comment on attachment 419051 [details]
Patch

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

r-, this is a good start!  I have some code layout/organization comments, as well as lots of style comments (I'd suggest familiarizing with <https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide>), but I think the core logic/UI is solid.  Please let me know if anything is confusing :)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:37
> +    async show(type, config)

`DOM.showGridOverlay` requires a `DOM.NodeId`, so we should match that here as well
```
    async show (overlayType, domNode, options = {})
    {
        console.assert(Object.values(WI.OverlayManager.OverlayType).includes(overlayType), overlayType);
        console.assert(domNode instanceof WI.DOMNode, domNode);
```
where `options` would contain the remaining optional parameters listed in the protocol

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:41
> +        let overlayIndex = this._shownOverlays.findIndex((overlay) => { return overlay.type === type && overlay.nodeId === config.nodeId; });

Style: either drop the `{` and `}` and `return` or put the function body onto separate lines
```
    let overlayIndex = this._shownOverlays.findIndex((overlay) => overlay.type === overlayType && overlay.domNode === domNode);
```

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:45
> +            if (Object.shallowEqual({type, ...config}, overlay))

While I appreciate this check, is there ever a situation right now where we pass the exact same values?  If it is, assuming the backend doesn't error if the exact same values are passed, I'd just make this into a `console.assert` as it wouldn't cause problems but is perhaps something that we should avoid/fix.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:51
> +        let domNode = WI.domManager.nodeForId(config.nodeId);
> +        if (!domNode)
> +            return Promise.reject(`Cannot show overlay, node is missing for id ${config.nodeId}`);

I would strongly suggest that you store the actual `WI.DOMNode` instead of the ID wherever possible as it'll give you more flexibility (and avoid having to repeatedly call `WI.domManager.nodeForId`).

Also, by passing a `WI.DOMNode` you know for a fact that you have something valid (you'd also want to check `domNode.destroyed` though) and wouldn't need these extra checks.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:58
> +        // Overwrite the existing overlay object if already defined or create a new one.
> +        overlay = {type, ...config};

Rather than have one giant list that needs to be searched for both `WI.DOMNode` and `WI.OverlayManager.OverlayType`, can we create a separate `Map` for each `WI.OverlayManager.OverlayType`?  As an example, you could have `_gridOverlayForNode` which you'd use if the given `overlayType === WI.OverlayManager.OverlayType.Grid`.  That way you wouldn't have to do as much searching when showing/hiding non-grid overlays and could avoid having to do things based off index (the key would be the `WI.DOMNode`).

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:64
> +         // FIXME (rcaliman): Set pending promise and wait for it to settle before calling another to prevent race conditions.

oops?

Also, the Web Inspector protocol is guaranteed to resolve in the order in which things are called, so calling two `DOM.showGridOverlay` where the second is invoked before the first resolves is not a problem.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:70
> +    async hide(type, config)

ditto (:37)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:111
> +    _getShowOverlayMethod(type, domNode) {

This type of indirection is not usually something we do as it makes it harder for code to be followed and searched.  Since this is only used once, I'd move the `switch` into `async show`.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:116
> +            // FIXME (rcaliman): Move contents of DOMNode.showGridOverlay() here and obsolete that.

oops?

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:124
> +    _getHideOverlayMethod(type, domNode) {

ditto (:111) with `async hide`

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:143
> +WI.OverlayManager.Type = {

NIT: I'd call this `OverlayType` so that it's clear that the type is referring to the overlay and not the overlay manager.  My initial thought when i saw this was that you'd have a different `WI.OverlayManager` per type instead of one global `WI.OverlayManager`.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:144
> +    Grid: "Grid",

We usually lowercase enum values (unless they match an uppercase value in the protocol, in which case this should have a comment indicating that).

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.css:26
> +.grid-style-section > .content {

These classes do not correspond to anything created in `GridDetailsSectionRow.js`.  We should only be using styles present there.

This rule should probably be moved to `LayoutDetailsSidebarPanel.css` instead.

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.css:27
> +    font-size: 11px;

Is the inherited `font-size` too large?  Can we put this in `.details-section > .row.css-grid` instead so that if other things are added to `.grid-style-section > .content` then they are not also affected?

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.css:30
> +.grid-style-section .css-grid {

I'd suggest using a more specific selector like `.details-section > .row.css-grid` so that we avoid conflicts (especially with such a general class like `.css-grid`).

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.css:31
> +    margin: 0 4px 4px 6px;

Please use LTR/RTL agnostic properties if the left and right values differ
```
    margin-inline-start: 6px;
    margin-inline-end: 4px;
    margin-bottom: 4px;
```

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:26
> +WI.GridDetailsSectionRow = class GridDetailsSectionRow extends WI.DetailsSectionRow {

Style: braces on new lines

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:29
> +        super("No CSS grids on page.");

Style: when passing non-obvious constants to a function as an argument, we usually prefer to create a `const` local variable with the name of the parameter as the name of the variable so that it's clear what it's used for
```
    const emptyMessage = WI.UIString("No CSS grid contexts.");
    super(emptyMessage);
```

Also, please use `WI.UIString` for text that is shown in the UI.

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:32
> +        this._linkListElement = document.createElement("ul");

NIT: I think just `_listElement` is fine

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:34
> +        this.element.appendChild(this._linkListElement);

NIT: we often cut down on line numbers by combining this with the same line as `document.createElement`
```
    this._listElement = this.element.appendChild(document.createElement("ul"));
```

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:36
> +        this.element.addEventListener("change", this);

Please don't use the "legacy" event listener object "system" in favor of explicitly adding event listeners for that event onto the elements that dispatch them.

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:38
> +        this._gridNodeList = [];

NIT: while there's nothing wrong with this, we usually avoid words like "List" instead preferring just to use the plural form of the items expected to be held (e.g. `_gridNodes` or `_nodesWithGridContext`)

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:42
> +        this._onOverlayStateChanged = this._onOverlayStateChanged.bind(this);
> +
> +        WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayShown, this._onOverlayStateChanged);
> +        WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayHidden, this._onOverlayStateChanged);

Web Inspector's custom event listener system doesn't require (and actively discourages) from using `.bind`.  In fact, this should have triggered some `console.assert` in inspector^2 since you didn't supply a 3rd argument (`thisObject`).
```
    WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayShown, this._handleOverlayStateChanged, this);
    WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayHidden, this._handleOverlayStateChanged, this);
```

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:45
> +    set gridNodeList(nodeList) {

ditto (:26)

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:46
> +        // FIXME (rcaliman): check for different node ids and skip relayout if not required

oops?

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:51
> +    get gridNodeList() {

ditto (:26)

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:55
> +    handleEvent(event)

ditto (:36)

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:63
> +    _onOverlayStateChanged(event) {

NIT: we prefer to prefix event handlers with `_handle` instead of `_on`

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:68
> +        let checkboxElement = this._linkListElement.querySelector(`input[value="${event.data.nodeId}"]`);

We try to avoid using any DOM querying in Web Inspector code as much as possible.  I'd suggest having a `this._checkboxElementForNode = new Map` in the constructor that's populated by `this._checkboxElementForNode.set(domNode.id, checkboxElement)` and can then be queried later by `this._checkboxElementForNode.get(event.data.nodeId)`.

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:76
> +        if (event.type === WI.OverlayManager.Event.OverlayShown)
> +            checkboxElement.checked = true;
> +
> +        if (event.type === WI.OverlayManager.Event.OverlayHidden)
> +            checkboxElement.checked = false;

Can we simplify this as just
```
    checkboxElement.checked = event.type === WI.OverlayManager.Event.OverlayShown;
```
or are you expecting this handler to deal with more events in the future (if that's the case then I'd suggest creating specific handlers for each so that the logic is more straightfoward).

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:86
> +            let idAttribute = `toggle-grid-overlay-${domNode.id}`;

Instead of polluting the `id` map, one trick we often use is to nest the form element inside the `<label>`, so I'd do this instead
```
    let itemElement = this._listElement.appendChild(document.createElement("li"));

    let labelElement = itemElement.appendChild(document.createElement("label"));

    let checkboxElement = labelElement.appendChild(document.createElement("input"));
    checkboxElement.type = "checkbox";
    checkboxElement.checked = shownOverlayNodes.has(domNode);

    labelElement.appendChild(WI.linkifyNodeReference(domNode));

    ...

    checkboxElement.addEventListener("change", (event) => {
        if (checkboxElement.checked)
            WI.overlayManager.show(WI.OverlayManager.Type.Grid, domNode, {color: swatch.value});
        else
            WI.overlayManager.hide(WI.OverlayManager.Type.Grid, domNode);
    });

    swatch.addEventListener(WI.InlineSwatch.Event.ValueChanged, function(event) {
        if (checkboxElement.checked)
            WI.overlayManager.show(WI.OverlayManager.Type.Grid, domNode, {color: this.value});
    }, swatch);
```

> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:101
> +            let readOnly = true;

Style: `const`

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:35
> +        this._gridListRefresh = this._gridListRefresh.bind(this);

ditto (GridDetailsSectionRow.js:39)

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:104
> +        let gridRow = new WI.GridDetailsSectionRow();

Style: you can drop `()` when constructing if there are no arguments

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:106
> +        let gridSection = new WI.DetailsSection("grid-style-section", WI.UIString("Grid", "Grid @ Styles sidebar", "CSS Grid layout section name"), [gridGroup]);

NIT: the key really should say `Grid @ Elements details sidebar` since this has nothing to do with the Styles sidebar/panel

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:109
> +        this.gridDetailsSectionRow = gridRow;

Should this be "private"?  `this>_gridDetailsSectionRow`

Also, please just assign/initialize it directly instead of creating a local variable.

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:110
> +        this._addEventListenersForGridListRefresh();

This should be moved to `attached` so that the event listeners are re-added after the Layout is re-shown.

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:144
> +    _addEventListenersForGridListRefresh()

IMO this could just be inlined in `attached` instead of being a separate function

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:153
> +    _removeEventListenersForGridListRefresh()

ditto (:144) with `detached`

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:162
> +    _gridListRefresh()

NIT: `_refreshGridList`?

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:166
> +        this._getAllGridDOMNodes().then(gridNodeList => {

NIT: you could make this `async _refreshGridList` so that you can use `await`
```
    let gridNodes = await this._getAllGridDOMNodes();
    this._gridDetailsSectionRow.gridNodeList = gridNodes;
```

Style: we always wrap the parameters list of arrow functions in `(` `)` even if there's only one parameter

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:171
> +    // Returns an array of WI.DOMNode instances that have `display: grid` or `display: inline-grid`.

These kinds of comments are kinda redundant when you look at the name of the function.  I think you can remove it.

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:178
> +        let ids = await doc.querySelectorAll("*");

I know you said that this isn't expected to be here forever, but please put a FIXME comment with a bug URL so that we know that this needs to be changed.

Also, I'd use a more descriptive name like `nodeIDs`.

[Razvan Caliman]

Created attachment 419173 [details]
Patch

[Razvan Caliman]

Created attachment 419507 [details]
Patch

[Razvan Caliman]

Created attachment 419508 [details]
[Image] With patch applied

[Razvan Caliman]

Comment on attachment 419507 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:63
> +    async _show(overlayType, domNode, options = {})
> +    {
> +        console.assert(Object.values(WI.OverlayManager.OverlayType).includes(overlayType), overlayType);
> +        console.assert(domNode instanceof WI.DOMNode, domNode);
> +
> +        let overlayMap = this._overlayMapForOverlayTypeMap.get(overlayType);
> +        let overlay = {overlayType, domNode, ...options};
> +        overlayMap.set(domNode, overlay);
> +        this.dispatchEventToListeners(WI.OverlayManager.Event.OverlayShown, overlay);
> +    }
> +
> +    async _hide(overlayType, domNode)
> +    {
> +        console.assert(Object.values(WI.OverlayManager.OverlayType).includes(overlayType), overlayType);
> +        console.assert(domNode instanceof WI.DOMNode, domNode);
> +
> +        let overlayMap = this._overlayMapForOverlayTypeMap.get(overlayType);
> +        let overlay = overlayMap.get(domNode);
> +        if (!overlay)
> +            return;
> +
> +        overlayMap.delete(domNode);
> +        this.dispatchEventToListeners(WI.OverlayManager.Event.OverlayHidden, overlay);
> +    }

_show() and _hide() are reduced to generic methods for storing/releasing the DOMNode => overlay mapping and dispatching events.
Now we manage only Grid overlays, but in the future we'll likely have others that will reuse these.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:88
> +        let target = WI.assumingMainTarget();
> +        let options = {
> +            nodeId: domNode.id,
> +            gridColor: color.toProtocol(),
> +            showLineNames: !!showLineNames,
> +            showLineNumbers: !!showLineNumbers,
> +            showExtendedGridLines: !!showExtendedGridLines,
> +            showTrackSizes: !!showTrackSizes,
> +            showAreaNames: !!showAreaNames,
> +        };
> +        target.DOMAgent.showGridOverlay.invoke(options);

DOMNode.showGridOverlay() and DOMNode.hideGridOverlay() will be removed shortly. Their contents are migrated here (and to :100)
The methods were added for engineering convenience, but should be removed soon after this lands.

OverlayManger must mediate showing/hiding in order to maintain a centralized state of visible overlays & fire events so clients react accordingly (checkboxes in Layout panel and upcoming Grid badges in DOM tree).

[Patrick Angle]

Comment on attachment 419507 [details]
Patch

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

Not a review, but some thoughts/questions…

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:94
> +        WI.domManager.removeEventListener(WI.DOMManager.Event.NodeInserted, this._refreshGridNodeList, this);
> +        WI.domManager.removeEventListener(WI.DOMManager.Event.NodeRemoved, this._refreshGridNodeList, this);

Do these really need to refresh the entire list, or could they work similarly to `_handleLayoutContextTypeChanged` where you check the layout context type and add/remove only if necessary?

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:164
> +        this._gridNodeList = new Set(WI.domManager.getNodes().filter(domNode => domNode.layoutContextType === WI.DOMNode.LayoutContextType.Grid));

I wonder if it would make more sense for `DOMManager` to have a `getNodesWithLayoutContextType(contextType)` function instead of `getNodes` which in turn leaves filtering to the view and any future views/call sites that want to filter by layout context (flex, for example, and some tests in the patch I started yesterday).

[Razvan Caliman]

Comment on attachment 419507 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:94
>> +        WI.domManager.removeEventListener(WI.DOMManager.Event.NodeRemoved, this._refreshGridNodeList, this);
> 
> Do these really need to refresh the entire list, or could they work similarly to `_handleLayoutContextTypeChanged` where you check the layout context type and add/remove only if necessary?

We should support the case where the insertion of a sibling or an ancestor causes grid context to become available on a node that's already present in the DOM.

Perhaps a rare case, but not impossible: adding an h2 causes an already present div to become a grid.

```
h2 + div {
  display: grid;
}
```

Or maybe `_handleLayoutContextTypeChanged()` supports this class of use cases already?

>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:164
>> +        this._gridNodeList = new Set(WI.domManager.getNodes().filter(domNode => domNode.layoutContextType === WI.DOMNode.LayoutContextType.Grid));
> 
> I wonder if it would make more sense for `DOMManager` to have a `getNodesWithLayoutContextType(contextType)` function instead of `getNodes` which in turn leaves filtering to the view and any future views/call sites that want to filter by layout context (flex, for example, and some tests in the patch I started yesterday).

Yes, we can do that. Sounds more flexible. Even better if there are other consumers for it already.

[Devin Rousso]

Comment on attachment 419507 [details]
Patch

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

r- due to performance concerns in LayoutDetailsSidebarPanel.js, but otherwise mostly just Style and naming :)

Making good progress!

> Source/WebInspectorUI/ChangeLog:9
> +        Show a list of CSS Grid containers in the Layout sidebar panel of the Elements tab.

NIT: we capitalize `"Tab"` when it's used alongside the name of the tab (e.g. "tabs" vs "Elements Tab")

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:707
> +

no newline

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:709
> +localizedStrings["Grid overlays heading @ Layout Sidebar Section"] = "Grid overlays";

NIT: we usually only put the string to be shown in the UI before the "@" (e.g. "Grid Overlays @ Layout Sidebar Section Header")

Also, I think this is missing a comment based on the `WI.UIString` call with this key.  Rather than edit this file manually, you can just run `./Tools/Scripts/extract-localizable-js-strings --utf8 Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js Source/WebInspectorUI/UserInterface' and have it all done for you :)

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:186
> +    getNodes() {

Style: `{` on next line

Why not make this an actual JS getter (i.e. `get nodes()`)?

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:703
> -            .then(({eventNames}) => new Set(eventNames));
> +                .then(({eventNames}) => new Set(eventNames));

while I appreciate these style fixes, we usually don't do these kind of drive-by fixes as it makes it harder to `git blame` a line and find out when it was added

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:778
> -            if (!WI.debuggerManager.breakpointsDisabledTemporarily)
> -                WI.debuggerManager.breakpointsEnabled = true;
> +        if (!WI.debuggerManager.breakpointsDisabledTemporarily)
> +            WI.debuggerManager.breakpointsEnabled = true;

ditto (:+703)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:32
> +        this._gridOverlayForNodeMap = new Map;

It's really odd to have a `Map` saved to a variable but also be present inside another `Map` (which is also a member variable).  Can we just replace `this._overlayMapForOverlayTypeMap` with a `switch`?
```
    _overlayForOverlayType(overlayType, domNode)
    {
        switch (overlayType) {
        case WI.OverlayManager.OverlayType.Grid:
            return this._gridOverlayForNodeMap.get(domNode);
        }

        console.assert(false); // not reached
        return null;
    }
```

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:38
> +    // Private

Style: `// Private` should go below `// Public`

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:45
> +        let overlayMap = this._overlayMapForOverlayTypeMap.get(overlayType);

ditto (:+32)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:46
> +        let overlay = {overlayType, domNode, ...options};

I feel like we should create a `WI.OverlayManager.OverlayConfiguration` class that has this logic so that we don't need to pass around plain objects and do (potentially colliding) object spreads.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:56
> +        let overlayMap = this._overlayMapForOverlayTypeMap.get(overlayType);

ditto (:+32)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:57
> +        let overlay = overlayMap.get(domNode);

We have a utility for `Map.prototype.take` which does a `get` and `delete` in one call :)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:65
> +    // Grid Overlay

Style: this should just be `'// Public`

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:73
> +        console.assert(color instanceof WI.Color, color);

Since `color` is part of the implicit `options` object, it's not required, so this should be accepting of that.
```
    console.assert(!color || color instanceof WI.Color, color);
```

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:76
> +        if (!color)
> +            color = WI.Color.fromString("magenta"); // fallback color

```
    color ||= WI.Color.fromString("magenta");
```

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:79
> +        let options = {

NIT: a more accurate name for this would be `arguments`

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:90
> +        target.DOMAgent.showGridOverlay.invoke(options);
> +
> +        this._show(WI.OverlayManager.OverlayType.Grid, domNode, options);

Does this need an `await` somewhere?  I ask because you're returning a rejected `Promise `above and we prefer to have our functions be consistent in their return values.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:103
> +        target.DOMAgent.hideGridOverlay(domNode.id);
> +
> +        this._hide(WI.OverlayManager.OverlayType.Grid, domNode);

ditto (:+90)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:106
> +    toggleGridOverlay(domNode, options = {})

This doesn't appear to be called anywhere.  Do we need it?

Generally we try to reflect the state of things based on what's shown in the UI, so for example if somehow a checkbox becomes out of sync with the backend we'd rather send the same message twice than have the backend not show an overlay when the checkbox is checked (or vice versa), so we usually avoid "toggle" kinds of things and instead prefer explicit "enable" and "disable".

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:114
> +    getNodesForGridOverlay()

Why not make this an actual JS getter (i.e. `get nodesWithGridOverlay()`)?

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:116
> +        return this._gridOverlayForNodeMap.keys();

NIT: this will return an interator, not an array/set.  I notice that you wrap it in `new Set` at the callsite, but that shouldn't be necessary since `Map` keys cannot be repeated.  Perhaps we should just `Array.from` here so that callers can `filter`/`map`/etc.?

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:125
> +WI.OverlayManager.OverlayType = {

based on my comment on CSSGridSection.js:+103, this will not be used outside of `WI.OverlayManager`, so you could make this "private" (`WI.OverlayManager._OverlayType`) as it really is a bookkeeping detail of `WI.OverlayManager` and perhaps not anything anyone outside of `WI.OverlayManager` would ever need to use

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.css:33
> +    margin-block-start: 1em;
> +    margin-block-end: 0.75em;

we usually don't use `em` units

also, please use `margin-top` and `margin-bottom` since Web Inspector only supports RTL not vertical writing modes

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

2021

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:34
> +        this._gridNodeList = new Set;

`List` should not be used when the value is a `Set`

Either make it `_gridNodeSet` or just drop it altogether as `_gridNodes` (this implies some form of flat collection, which is usually good enough)

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:38
> +        WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayShown, this._handleOverlayStateChanged, this);
> +        WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayHidden, this._handleOverlayStateChanged, this);

please move these to `initialLayout`

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:43
> +    set gridNodeList(nodeList) {

Style: `{` on new line

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:48
> +    get gridNodeList() {

Style: `{` on new line

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:68
> +        this._listElement.removeChildren();

Style: newline after

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:69
> +        let shownOverlayNodes = new Set(WI.overlayManager.getNodesForGridOverlay());

see comments on OverlayManager.js:+116

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:72
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:90
> +            const readOnly = false;
> +            let swatch = new WI.InlineSwatch(WI.InlineSwatch.Type.Color, WI.Color.fromString("magenta"), readOnly);

it's not necessary to provide falsy arguments to functions/constructors if they are trailing arguments as falsy (specifically `undefined`) is implicitly used if the argument is not provided

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:102
> +    _handleOverlayStateChanged(event) {

Style: `{` on new line

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:104
> +        // Only care about events for Grid overlays, ignore all else.
> +        if (event.data.overlayType !== WI.OverlayManager.OverlayType.Grid)

Do we ever think there will be a scenario where a listener will care about more than one type of overlay at the same time?  I would think not.

Based on this, I'd suggest making the `WI.OverlayManager.Event` more specific to the type of overlay (e.g. `WI.OverlayManager.Event.GridOverlayShown`) so that this can be avoided (not to mention it'll be more performant if more overlays are introduced because this won't have to listen for changes to those).

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.css:29
> +    margin-block-end: 4px;

please use `margin-bottom` since Web Inspector only supports RTL not vertical writing modes

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:33
> +        this._gridNodeList = new Set;

ditto (CSSGridSection.js:+34)

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:86
> +        WI.domManager.addEventListener(WI.DOMManager.Event.NodeInserted, this._refreshGridNodeList, this);
> +        WI.domManager.addEventListener(WI.DOMManager.Event.NodeRemoved, this._refreshGridNodeList, this);
> +        WI.domManager.addEventListener(WI.DOMManager.Event.DocumentUpdated, this._refreshGridNodeList, this);

This is a really inefficient system.  Not only are you listening to _every_ node addition/removal, but you're also iterating over the _entire_ list of instrumented nodes each time this happens.  This should be optimized.

I think you should modify `WI.DOMNode` to call `this.layoutContextType = null;` whenever it's removed from the DOM tree (meaning that it'll no longer be rendered and therefore can be assumed to not have a layout context), which for most nodes would be a no-op since their `layoutContextType` was already `null` (i.e. not a grid context), meaning that all you'd need is the single `WI.DOMNode.Event.LayoutContextTypeChanged` listener added to `WI.DOMNode` to handle all modifications (but more importantly the nature of `WI.DOMNode.prototype.set layoutContextType` would effectively filter out most of the no-op operations).  Also, this would make it so that you'd only have to `add`/`delete` from `_gridNodes` inside `_handleLayoutContextTypeChanged` since you're specifically listening for changes from `WI.DOMNode.prototype.set layoutContextType`.  Note that you may need to make some minor modifications to `WI.DOMNode` to make this happen (e.g. instead of `console.assert` inside `WI.DOMNode.prototype.set layoutContextType` you'd want to actually `return` and the `_layoutContextType = ...` in the constructor should also invoke the setter so that `WI.DOMNode.Event.LayoutContextTypeChanged` is fired).  Also you would likely need to regenerate the `Set` inside `attached()` since this isn't listening to `WI.DOMNode.Event.LayoutContextTypeChanged` when not shown, but that's _far_ less often than what you have now so it's more OK :)

>>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:94
>>> +        WI.domManager.removeEventListener(WI.DOMManager.Event.NodeRemoved, this._refreshGridNodeList, this);
>> 
>> Do these really need to refresh the entire list, or could they work similarly to `_handleLayoutContextTypeChanged` where you check the layout context type and add/remove only if necessary?
> 
> We should support the case where the insertion of a sibling or an ancestor causes grid context to become available on a node that's already present in the DOM.
> 
> Perhaps a rare case, but not impossible: adding an h2 causes an already present div to become a grid.
> 
> ```
> h2 + div {
>   display: grid;
> }
> ```
> 
> Or maybe `_handleLayoutContextTypeChanged()` supports this class of use cases already?

+1 to @Patrick Angle's comment (also see my comment above)

BTW if a sibling is added that causes the node to become a grid context, I would think that that would be handled by `CSS.layoutContextTypeChanged`.  All you're really handling here is the case where a brand-new node is added or when the node (or ancestor) is fully removed from the DOM tree.

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:117
> +        this.addSubview(this._cssGridSection);
> +        cssGridRow.element.appendChild(this._cssGridSection.element);

you'll actually want to flip the order of these, as `WI.View.prototype.addSubview` has a special case for if the `WI.View` being added is already attached (see `WI.View.prototype.insertSubviewBefore`)

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:159
> +        this._cssGridSection.gridNodeList = this._gridNodeList;

Why can't the `WI.CSSGridSection` handle the logic of maintaining `_gridNodes`?  Are you expecting to have a `WI.CSSGridSection` that only shows a subset of grid nodes?  To be fair, if we expect to have special instrumentation for other layout contexts (e.g. `display: flex;`) it's possibly more efficient to have the listener here and have it provide a `Set` to the relevant `WI.CSSGridSection`/`WI.CSSFlexSection`/etc. based on the old and new context types, but I kinda doubt it. :/

[Razvan Caliman]

Comment on attachment 419507 [details]
Patch

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:709
>> +localizedStrings["Grid overlays heading @ Layout Sidebar Section"] = "Grid overlays";
> 
> NIT: we usually only put the string to be shown in the UI before the "@" (e.g. "Grid Overlays @ Layout Sidebar Section Header")
> 
> Also, I think this is missing a comment based on the `WI.UIString` call with this key.  Rather than edit this file manually, you can just run `./Tools/Scripts/extract-localizable-js-strings --utf8 Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js Source/WebInspectorUI/UserInterface' and have it all done for you :)

Neat! This is handy.

>> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:186
>> +    getNodes() {
> 
> Style: `{` on next line
> 
> Why not make this an actual JS getter (i.e. `get nodes()`)?

Replaced by `nodesWithLayoutContextType(layoutContextType)` introduced by Patrick's patch in https://bugs.webkit.org/show_bug.cgi?id=221449

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:32
>> +        this._gridOverlayForNodeMap = new Map;
> 
> It's really odd to have a `Map` saved to a variable but also be present inside another `Map` (which is also a member variable).  Can we just replace `this._overlayMapForOverlayTypeMap` with a `switch`?
> ```
>     _overlayForOverlayType(overlayType, domNode)
>     {
>         switch (overlayType) {
>         case WI.OverlayManager.OverlayType.Grid:
>             return this._gridOverlayForNodeMap.get(domNode);
>         }
> 
>         console.assert(false); // not reached
>         return null;
>     }
> ```

Coupled with comments from CSSGridSection.js:104 about using specific event names like `WI.OverlayManager.Event.GridOverlayShown` and from OverlayManager.js:125 about making the overlay type private, `WI.OverlayManager._OverlayType`, there are fewer and fewer reasons to have any generic methods within `OverlayManager`. There's also little point in having a generic getter for overlay maps if we only ever use just one.

I resorted to making all contents of OverlayManager be specific to grid overlays (removed `_show()` / _hide()` and moved contents into `showGridOverlay()` / `hideGridOverlay()`)

When we implement other overlays, I feel we might swing back to extracting some generic handlers. Been here before while working on other DevTools.

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:46
>> +        let overlay = {overlayType, domNode, ...options};
> 
> I feel like we should create a `WI.OverlayManager.OverlayConfiguration` class that has this logic so that we don't need to pass around plain objects and do (potentially colliding) object spreads.

See OverlayManager.js:32 
After making OverlayManager so specific, `overlay` is really just about grid overlays. If it's still a liability, I'd like to tackle this in a later patch, after landing overlay settings and configurable overlay colors patches.

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:57
>> +        let overlay = overlayMap.get(domNode);
> 
> We have a utility for `Map.prototype.take` which does a `get` and `delete` in one call :)

Neat!

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:79
>> +        let options = {
> 
> NIT: a more accurate name for this would be `arguments`

Can't use "arguments" in strict mode.

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:90
>> +        this._show(WI.OverlayManager.OverlayType.Grid, domNode, options);
> 
> Does this need an `await` somewhere?  I ask because you're returning a rejected `Promise `above and we prefer to have our functions be consistent in their return values.

Dropped _show() and _hide()

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:106
>> +    toggleGridOverlay(domNode, options = {})
> 
> This doesn't appear to be called anywhere.  Do we need it?
> 
> Generally we try to reflect the state of things based on what's shown in the UI, so for example if somehow a checkbox becomes out of sync with the backend we'd rather send the same message twice than have the backend not show an overlay when the checkbox is checked (or vice versa), so we usually avoid "toggle" kinds of things and instead prefer explicit "enable" and "disable".

I agree with the sentiment. 

`OverlayManager.toggleGridOverlay()` will come in handy when introducing Grid badges in the Elements tree view. Clicking a badge should toggle the overlay. The badges themselves will be style in response to the `GridOverlayShown`/`GridOverlayHidden` events (like checkboxes in `CSSGridSection`) or their corresponding nodes' presence in `OverlayManager.nodesWithGridOverlay` array. The toggle method is a convenience.

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:114
>> +    getNodesForGridOverlay()
> 
> Why not make this an actual JS getter (i.e. `get nodesWithGridOverlay()`)?

Good idea! Replaced with `get nodesWithGridOverlay()`

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:116
>> +        return this._gridOverlayForNodeMap.keys();
> 
> NIT: this will return an interator, not an array/set.  I notice that you wrap it in `new Set` at the callsite, but that shouldn't be necessary since `Map` keys cannot be repeated.  Perhaps we should just `Array.from` here so that callers can `filter`/`map`/etc.?

Wrapping with `new Set()` was for the convenience of calling `Set.has()`, but we can do `Array.from()` and `Array.includes()` as well.

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:125
>> +WI.OverlayManager.OverlayType = {
> 
> based on my comment on CSSGridSection.js:+103, this will not be used outside of `WI.OverlayManager`, so you could make this "private" (`WI.OverlayManager._OverlayType`) as it really is a bookkeeping detail of `WI.OverlayManager` and perhaps not anything anyone outside of `WI.OverlayManager` would ever need to use

Dropped altogether. See comment at OverlayManager.js:32

>> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:34
>> +        this._gridNodeList = new Set;
> 
> `List` should not be used when the value is a `Set`
> 
> Either make it `_gridNodeSet` or just drop it altogether as `_gridNodes` (this implies some form of flat collection, which is usually good enough)

Opted for `_gridNodeSet` in the name of clarity.

>> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:90
>> +            let swatch = new WI.InlineSwatch(WI.InlineSwatch.Type.Color, WI.Color.fromString("magenta"), readOnly);
> 
> it's not necessary to provide falsy arguments to functions/constructors if they are trailing arguments as falsy (specifically `undefined`) is implicitly used if the argument is not provided

Accidental leftover. Removed.

>> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:104
>> +        if (event.data.overlayType !== WI.OverlayManager.OverlayType.Grid)
> 
> Do we ever think there will be a scenario where a listener will care about more than one type of overlay at the same time?  I would think not.
> 
> Based on this, I'd suggest making the `WI.OverlayManager.Event` more specific to the type of overlay (e.g. `WI.OverlayManager.Event.GridOverlayShown`) so that this can be avoided (not to mention it'll be more performant if more overlays are introduced because this won't have to listen for changes to those).

Based on OverlayManager:32, switched to specific events `WI.OverlayManager.Event.GridOverlayShown` and `WI.OverlayManager.Event.GridOverlayHidden`.

Even with future overlays are introduced, unless they're high frequency show/hide overlays, the perf overhead should be negligible for generic listening and matching by type. 
But since we don't use overlay types anywhere else anymore, makes sense to drop the generic events in favor of the specific ones. Makes call sites cleaner at the expense of accumulating potential duplication in `OverlayManager`.

>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:86
>> +        WI.domManager.addEventListener(WI.DOMManager.Event.DocumentUpdated, this._refreshGridNodeList, this);
> 
> This is a really inefficient system.  Not only are you listening to _every_ node addition/removal, but you're also iterating over the _entire_ list of instrumented nodes each time this happens.  This should be optimized.
> 
> I think you should modify `WI.DOMNode` to call `this.layoutContextType = null;` whenever it's removed from the DOM tree (meaning that it'll no longer be rendered and therefore can be assumed to not have a layout context), which for most nodes would be a no-op since their `layoutContextType` was already `null` (i.e. not a grid context), meaning that all you'd need is the single `WI.DOMNode.Event.LayoutContextTypeChanged` listener added to `WI.DOMNode` to handle all modifications (but more importantly the nature of `WI.DOMNode.prototype.set layoutContextType` would effectively filter out most of the no-op operations).  Also, this would make it so that you'd only have to `add`/`delete` from `_gridNodes` inside `_handleLayoutContextTypeChanged` since you're specifically listening for changes from `WI.DOMNode.prototype.set layoutContextType`.  Note that you may need to make some minor modifications to `WI.DOMNode` to make this happen (e.g. instead of `console.assert` inside `WI.DOMNode.prototype.set layoutContextType` you'd want to actually `return` and the `_layoutContextType = ...` in the constructor should also invoke the setter so that `WI.DOMNode.Event.LayoutContextTypeChanged` is fired).  Also you would likely need to regenerate the `Set` inside `attached()` since this isn't listening to `WI.DOMNode.Event.LayoutContextTypeChanged` when not shown, but that's _far_ less often than what you have now so it's more OK :)

Implemented. Thank you for the advice!

>>>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:94
>>>> +        WI.domManager.removeEventListener(WI.DOMManager.Event.NodeRemoved, this._refreshGridNodeList, this);
>>> 
>>> Do these really need to refresh the entire list, or could they work similarly to `_handleLayoutContextTypeChanged` where you check the layout context type and add/remove only if necessary?
>> 
>> We should support the case where the insertion of a sibling or an ancestor causes grid context to become available on a node that's already present in the DOM.
>> 
>> Perhaps a rare case, but not impossible: adding an h2 causes an already present div to become a grid.
>> 
>> ```
>> h2 + div {
>>   display: grid;
>> }
>> ```
>> 
>> Or maybe `_handleLayoutContextTypeChanged()` supports this class of use cases already?
> 
> +1 to @Patrick Angle's comment (also see my comment above)
> 
> BTW if a sibling is added that causes the node to become a grid context, I would think that that would be handled by `CSS.layoutContextTypeChanged`.  All you're really handling here is the case where a brand-new node is added or when the node (or ancestor) is fully removed from the DOM tree.

Dropped listening to `WI.DOMManager.Event.NodeRemoved` / `WI.DOMManager.Event.NodeAdded` in favor of `WI.DOMNode.Event.LayoutContextTypeChanged` with changes suggested above.`

>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:159
>> +        this._cssGridSection.gridNodeList = this._gridNodeList;
> 
> Why can't the `WI.CSSGridSection` handle the logic of maintaining `_gridNodes`?  Are you expecting to have a `WI.CSSGridSection` that only shows a subset of grid nodes?  To be fair, if we expect to have special instrumentation for other layout contexts (e.g. `display: flex;`) it's possibly more efficient to have the listener here and have it provide a `Set` to the relevant `WI.CSSGridSection`/`WI.CSSFlexSection`/etc. based on the old and new context types, but I kinda doubt it. :/

Yes. There's no reason for this logic to reside outside of `CSSGridSection`. Moving it there.
There's no use case yet for showing subsets of grid nodes. Whenever flex containers get instrumented, we'll likely have a distinct section and UX for it anyway.

[Razvan Caliman]

Created attachment 419723 [details]
Patch

[Devin Rousso]

Comment on attachment 419507 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:79
>>> +        let options = {
>> 
>> NIT: a more accurate name for this would be `arguments`
> 
> Can't use "arguments" in strict mode.

Sorry yeah realized that after I think I usually use `commandArguments`.

>>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:106
>>> +    toggleGridOverlay(domNode, options = {})
>> 
>> This doesn't appear to be called anywhere.  Do we need it?
>> 
>> Generally we try to reflect the state of things based on what's shown in the UI, so for example if somehow a checkbox becomes out of sync with the backend we'd rather send the same message twice than have the backend not show an overlay when the checkbox is checked (or vice versa), so we usually avoid "toggle" kinds of things and instead prefer explicit "enable" and "disable".
> 
> I agree with the sentiment. 
> 
> `OverlayManager.toggleGridOverlay()` will come in handy when introducing Grid badges in the Elements tree view. Clicking a badge should toggle the overlay. The badges themselves will be style in response to the `GridOverlayShown`/`GridOverlayHidden` events (like checkboxes in `CSSGridSection`) or their corresponding nodes' presence in `OverlayManager.nodesWithGridOverlay` array. The toggle method is a convenience.

If they're like the checkboxes in `WI.CSSGridSection`, why not do like the checkboxes and `if (badgeView.enabled) { show() } else { hide() }`?  Having a `toggle` means that we open up the possibility for the exact issue I described above.

>>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:116
>>> +        return this._gridOverlayForNodeMap.keys();
>> 
>> NIT: this will return an interator, not an array/set.  I notice that you wrap it in `new Set` at the callsite, but that shouldn't be necessary since `Map` keys cannot be repeated.  Perhaps we should just `Array.from` here so that callers can `filter`/`map`/etc.?
> 
> Wrapping with `new Set()` was for the convenience of calling `Set.has()`, but we can do `Array.from()` and `Array.includes()` as well.

For what it's worth, I don't object at all to using a `Set` for `Set.prototype.has`.  My main point was that using a live interator can sometimes be error-prone and maybe we should flush it to some sort of collection (`Array` or `Set` or etc.).

>> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:38
>> +        WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayHidden, this._handleOverlayStateChanged, this);
> 
> please move these to `initialLayout`

I just realized that we may actually want these to be added in `attached` and removed in `detached` so that we don't have listeners being fired when this view is not visible.  My apologies for the misleading comment.

>>> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:104
>>> +        if (event.data.overlayType !== WI.OverlayManager.OverlayType.Grid)
>> 
>> Do we ever think there will be a scenario where a listener will care about more than one type of overlay at the same time?  I would think not.
>> 
>> Based on this, I'd suggest making the `WI.OverlayManager.Event` more specific to the type of overlay (e.g. `WI.OverlayManager.Event.GridOverlayShown`) so that this can be avoided (not to mention it'll be more performant if more overlays are introduced because this won't have to listen for changes to those).
> 
> Based on OverlayManager:32, switched to specific events `WI.OverlayManager.Event.GridOverlayShown` and `WI.OverlayManager.Event.GridOverlayHidden`.
> 
> Even with future overlays are introduced, unless they're high frequency show/hide overlays, the perf overhead should be negligible for generic listening and matching by type. 
> But since we don't use overlay types anywhere else anymore, makes sense to drop the generic events in favor of the specific ones. Makes call sites cleaner at the expense of accumulating potential duplication in `OverlayManager`.

I feel like I should mention that I am not trying to steer towards a direction of where `WI.OverlayManager` is only for grids.  I am totally fine and supportive of the idea of having a manager for _all_ types of overlays.  My main reason for pointing out these is that it's very easy to make specific things more generic, but it's often much harder to do the opposite.

I don't know of any cases right now where we'd need a single class to be notified of _all_ grids being shown/hidden, whereas i can think of a plethora of cases where something only needs to know about a specific type of overlay being shown/hidden (e.g. `WI.CSSGridSection` shouldn't care about flex overlays or regular DOM node overlays).  If that generic case does come around, it's very easy for us to make it more generic by just having that class add listeners on the `WI.OverlayManager` for all the events it cares about (see `WI.DOMDebuggerManager` or `WI.SourcesNavigationSidebarPanel` as examples of this), but until then we shouldn't be penalizing the specific cases by having them have to perform extra checks on their event handlers.

Thinking about it more, if we wanted to be the most specific, I think we should store the information about the shown overlays on `WI.DOMNode` as a `Map` of `WI.OverlayManager.OverlayType` as the key and some object (or maybe a `WI.OverlayConfiguration` class) as the value (frankly you're kinda already doing this with the `_gridOverlayForNodeMap` but instead of it being held by `WI.DOMNode` it's held by `WI.OverlayManager`).  This way, instead of a generic `WI.OverlayManager.Event.GridOverlayShown` that would require `WI.DOMTreeElement` to check "event.data.domNode === this.representedObject` (or something like that) we could skip that part and assume that if `WI.DOMNode.Event.GridOverlayShown` we can immediately create/enable the grid badge without having to perform any checks whatsoever.  Keep in mind that with the design of `WI.OverlayManager.Event.GridOverlayShown`, if you add that listener to every `WI.DOMTreeElement` for the purpose of updating the badges, then that means that _every_ `WI.DOMTreeElement` will be notified when any grid overlay is shown even if the grid overlay is not for the DOM node that matches that `WI.DOMTreeElement`, which can be _hugely_ wasteful (depending on how many nodes are currently visible in the Elements Tab).  I realize that this may not seem like it supports a generic use case, but due to the awesomeness of `WI.Object` any object that needs to listen to _all_ grid overlays being shown/hidden can attach their event listener to `WI.DOMNode` instead of a specific instance, meaning that this specific approach does still support a more generic use.

[Devin Rousso]

Comment on attachment 419723 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:430
> +                node.layoutContextType = null;

I'm actually not 100% sure if this is correct.  I wonder if it's possible for an item in `parent.children` to also be present in `payloads`, like if the protocol gets tripped up and we re-push the children of a parent node even though the DOM tree hasn't actually changed.

EDIT: Well actually, thinking about it more, `_setChildrenPayload` creates brand new `WI.DOMNode` so it's not like `node` will exist anymore anyways, so I think this really should be `node.markDestroyed()`.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:69
> +        return Promise.resolve(overlay);

Do we ever actually expect the caller of `WI.overlayManager.showGridOverlay` to use the returned `Promise`?  If not, I think we can just drop this (and the `Promise.reject`) above) entirely as it's basically already handled by the `WI.OverlayManager.Event.GridOverlayShown` event dispatch anyways.

Apologies if my previous mention in comment #19 wasn't clear.  I wasn't explicitly asking for you to add a return value but was more asking if one was needed and if not to remove the `Promise.reject`.  If you do remove it though, I'd suggest at least adding a `console.assert` before the early returns so that we can catch those cases and try to fix them as `domNode.destroyed` is probably something we shouldn't ever hit.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:88
> +        return Promise.resolve(overlay);

ditto (:69)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:102
> +    GridOverlayShown: "grid-overlay-shown",
> +    GridOverlayHidden: "grid-overlay-hidden",

please prefix the event name string with the name of the class dispatching that event so that there's less of a chance of collision with a similarly named event from another class
```
    GridOverlayShown: "overlay-manager-grid-overlay-shown",
    GridOverlayHidden: "overlay-manager-grid-overlay-hidden",
```

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:56
> +        this.layoutContextType = payload.layoutContextType;

NIT: I don't think this would be an issue right now given the code we have, but just to be safe I'd suggest moving this to the end of the constructor so that if anyone is listening for `WI.DOMNode.addEventListener(WI.DOMNode.Event.LayoutContextTypeChanged, ...)` then they won't be notified with an `event.target` in the middle of its constructor call (i.e. some of the `WI.DOMNode`'s state might not be fully initialized yet).

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:979
> +            node.layoutContextType = null;

ditto (DOMManager.js:+430)

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.css:34
> +    font-weight: var(--sorted-header-font-weight);

This is using a badly named variable (the `WI.CSSGridSection` has nothing to do with "sorted headers" which is a `WI.Table` concept).  Either rename the variable to something more generic or just use `bold` (the other variable has different styles for pre-BigSur vs BigSur, which I don't think you want here).

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.css:35
> +    color: var(--text-color)

Style: missing `;`

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:42
> +        WI.DOMNode.addEventListener(WI.DOMNode.Event.LayoutContextTypeChanged, this._handleLayoutContextTypeChanged, this);

please add a `super.attached()` before

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:48
> +        WI.DOMNode.removeEventListener(WI.DOMNode.Event.LayoutContextTypeChanged, this._handleLayoutContextTypeChanged, this);

please add a `super.detached()` after

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:53
> +        let listHeading = this.element.appendChild(document.createElement("h2"));

please add a `super.initialLayout()` before

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:61
> +        WI.overlayManager.addEventListener(WI.OverlayManager.Event.GridOverlayShown, this._handleGridOverlayStateChanged, this);
> +        WI.overlayManager.addEventListener(WI.OverlayManager.Event.GridOverlayHidden, this._handleGridOverlayStateChanged, this);

Please see comment #22.  Apologies again if I mislead you.

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:125
> +        this.needsLayout();

Is this needed?  `_refreshGridNodeSet` is only called in `attached`, which is called when this is added to `WI.View` (or some parent does the same) so it should already have be in a `layoutPending` state.

[Razvan Caliman]

Comment on attachment 419507 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:106
>>>> +    toggleGridOverlay(domNode, options = {})
>>> 
>>> This doesn't appear to be called anywhere.  Do we need it?
>>> 
>>> Generally we try to reflect the state of things based on what's shown in the UI, so for example if somehow a checkbox becomes out of sync with the backend we'd rather send the same message twice than have the backend not show an overlay when the checkbox is checked (or vice versa), so we usually avoid "toggle" kinds of things and instead prefer explicit "enable" and "disable".
>> 
>> I agree with the sentiment. 
>> 
>> `OverlayManager.toggleGridOverlay()` will come in handy when introducing Grid badges in the Elements tree view. Clicking a badge should toggle the overlay. The badges themselves will be style in response to the `GridOverlayShown`/`GridOverlayHidden` events (like checkboxes in `CSSGridSection`) or their corresponding nodes' presence in `OverlayManager.nodesWithGridOverlay` array. The toggle method is a convenience.
> 
> If they're like the checkboxes in `WI.CSSGridSection`, why not do like the checkboxes and `if (badgeView.enabled) { show() } else { hide() }`?  Having a `toggle` means that we open up the possibility for the exact issue I described above.

Fair enough. The building blocks are there clients to handle their own checks and toggle between the two calls. I'll remove the method `toggleGridOverlay()`

>>>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:116
>>>> +        return this._gridOverlayForNodeMap.keys();
>>> 
>>> NIT: this will return an interator, not an array/set.  I notice that you wrap it in `new Set` at the callsite, but that shouldn't be necessary since `Map` keys cannot be repeated.  Perhaps we should just `Array.from` here so that callers can `filter`/`map`/etc.?
>> 
>> Wrapping with `new Set()` was for the convenience of calling `Set.has()`, but we can do `Array.from()` and `Array.includes()` as well.
> 
> For what it's worth, I don't object at all to using a `Set` for `Set.prototype.has`.  My main point was that using a live interator can sometimes be error-prone and maybe we should flush it to some sort of collection (`Array` or `Set` or etc.).

Got it! I think we can keep the `Array.includes()`call in this case. 
I Didn't know that passing around iterators was error-prone.

>>> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:38
>>> +        WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayHidden, this._handleOverlayStateChanged, this);
>> 
>> please move these to `initialLayout`
> 
> I just realized that we may actually want these to be added in `attached` and removed in `detached` so that we don't have listeners being fired when this view is not visible.  My apologies for the misleading comment.

Yeah, that's a good idea. Moved to attached/detached in the next patch.

>>>> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:104
>>>> +        if (event.data.overlayType !== WI.OverlayManager.OverlayType.Grid)
>>> 
>>> Do we ever think there will be a scenario where a listener will care about more than one type of overlay at the same time?  I would think not.
>>> 
>>> Based on this, I'd suggest making the `WI.OverlayManager.Event` more specific to the type of overlay (e.g. `WI.OverlayManager.Event.GridOverlayShown`) so that this can be avoided (not to mention it'll be more performant if more overlays are introduced because this won't have to listen for changes to those).
>> 
>> Based on OverlayManager:32, switched to specific events `WI.OverlayManager.Event.GridOverlayShown` and `WI.OverlayManager.Event.GridOverlayHidden`.
>> 
>> Even with future overlays are introduced, unless they're high frequency show/hide overlays, the perf overhead should be negligible for generic listening and matching by type. 
>> But since we don't use overlay types anywhere else anymore, makes sense to drop the generic events in favor of the specific ones. Makes call sites cleaner at the expense of accumulating potential duplication in `OverlayManager`.
> 
> I feel like I should mention that I am not trying to steer towards a direction of where `WI.OverlayManager` is only for grids.  I am totally fine and supportive of the idea of having a manager for _all_ types of overlays.  My main reason for pointing out these is that it's very easy to make specific things more generic, but it's often much harder to do the opposite.
> 
> I don't know of any cases right now where we'd need a single class to be notified of _all_ grids being shown/hidden, whereas i can think of a plethora of cases where something only needs to know about a specific type of overlay being shown/hidden (e.g. `WI.CSSGridSection` shouldn't care about flex overlays or regular DOM node overlays).  If that generic case does come around, it's very easy for us to make it more generic by just having that class add listeners on the `WI.OverlayManager` for all the events it cares about (see `WI.DOMDebuggerManager` or `WI.SourcesNavigationSidebarPanel` as examples of this), but until then we shouldn't be penalizing the specific cases by having them have to perform extra checks on their event handlers.
> 
> Thinking about it more, if we wanted to be the most specific, I think we should store the information about the shown overlays on `WI.DOMNode` as a `Map` of `WI.OverlayManager.OverlayType` as the key and some object (or maybe a `WI.OverlayConfiguration` class) as the value (frankly you're kinda already doing this with the `_gridOverlayForNodeMap` but instead of it being held by `WI.DOMNode` it's held by `WI.OverlayManager`).  This way, instead of a generic `WI.OverlayManager.Event.GridOverlayShown` that would require `WI.DOMTreeElement` to check "event.data.domNode === this.representedObject` (or something like that) we could skip that part and assume that if `WI.DOMNode.Event.GridOverlayShown` we can immediately create/enable the grid badge without having to perform any checks whatsoever.  Keep in mind that with the design of `WI.OverlayManager.Event.GridOverlayShown`, if you add that listener to every `WI.DOMTreeElement` for the purpose of updating the badges, then that means that _every_ `WI.DOMTreeElement` will be notified when any grid overlay is shown even if the grid overlay is not for the DOM node that matches that `WI.DOMTreeElement`, which can be _hugely_ wasteful (depending on how many nodes are currently visible in the Elements Tab).  I realize that this may not seem like it supports a generic use case, but due to the awesomeness of `WI.Object` any object that needs to listen to _all_ grid overlays being shown/hidden can attach their event listener to `WI.DOMNode` instead of a specific instance, meaning that this specific approach does still support a more generic use.

Thank you for the clarification! Makes sense. We'll make it generic when needed later.

Regarding the last part, moving the source of truth for shown grid overlays & events, if you feel strongly about it, we could follow-up in a separate patch. 

This patch is a blocker for other team members now. If is satisfactory as-is, I'd like to get it landed sooner to unblock them. What do you think?

[Razvan Caliman]

Comment on attachment 419723 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:430
>> +                node.layoutContextType = null;
> 
> I'm actually not 100% sure if this is correct.  I wonder if it's possible for an item in `parent.children` to also be present in `payloads`, like if the protocol gets tripped up and we re-push the children of a parent node even though the DOM tree hasn't actually changed.
> 
> EDIT: Well actually, thinking about it more, `_setChildrenPayload` creates brand new `WI.DOMNode` so it's not like `node` will exist anymore anyways, so I think this really should be `node.markDestroyed()`.

`node.markDestroyed()` doesn't do anything to flip the `layoutContextType`. If calling `node.markDestroyed()` is needed for some other effects, I'll add the flag flip to it so our desired behavior continues to happen.

```diff
    markDestroyed()
    {
        console.assert(!this._destroyed, this);
+       this.layoutContextType = null;
        this._destroyed = true;
    }
```

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:69
>> +        return Promise.resolve(overlay);
> 
> Do we ever actually expect the caller of `WI.overlayManager.showGridOverlay` to use the returned `Promise`?  If not, I think we can just drop this (and the `Promise.reject`) above) entirely as it's basically already handled by the `WI.OverlayManager.Event.GridOverlayShown` event dispatch anyways.
> 
> Apologies if my previous mention in comment #19 wasn't clear.  I wasn't explicitly asking for you to add a return value but was more asking if one was needed and if not to remove the `Promise.reject`.  If you do remove it though, I'd suggest at least adding a `console.assert` before the early returns so that we can catch those cases and try to fix them as `domNode.destroyed` is probably something we shouldn't ever hit.

We don't expect callsites to wait for `showGridOverlay()` or `hideGridOverlay()`. Removing the rejections is fine.

>> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:42
>> +        WI.DOMNode.addEventListener(WI.DOMNode.Event.LayoutContextTypeChanged, this._handleLayoutContextTypeChanged, this);
> 
> please add a `super.attached()` before

Doh! Thanks for catching this!

>> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:125
>> +        this.needsLayout();
> 
> Is this needed?  `_refreshGridNodeSet` is only called in `attached`, which is called when this is added to `WI.View` (or some parent does the same) so it should already have be in a `layoutPending` state.

Leftover from a time when this was a handler for multiple events. Removed now.
Also, didn't know about `layoutPending` state.

[Razvan Caliman]

Created attachment 419761 [details]
Patch

[Devin Rousso]

Comment on attachment 419723 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:430
>>> +                node.layoutContextType = null;
>> 
>> I'm actually not 100% sure if this is correct.  I wonder if it's possible for an item in `parent.children` to also be present in `payloads`, like if the protocol gets tripped up and we re-push the children of a parent node even though the DOM tree hasn't actually changed.
>> 
>> EDIT: Well actually, thinking about it more, `_setChildrenPayload` creates brand new `WI.DOMNode` so it's not like `node` will exist anymore anyways, so I think this really should be `node.markDestroyed()`.
> 
> `node.markDestroyed()` doesn't do anything to flip the `layoutContextType`. If calling `node.markDestroyed()` is needed for some other effects, I'll add the flag flip to it so our desired behavior continues to happen.
> 
> ```diff
>     markDestroyed()
>     {
>         console.assert(!this._destroyed, this);
> +       this.layoutContextType = null;
>         this._destroyed = true;
>     }
> ```

ah yes sorry that was what i was implying/suggesting (although I'd move the `this.layoutContextType` after the `this._destroyed = true;` so that any listeners of `WI.DOMNode.Event.LayoutContextTypeChanged` would be able to see that it's now `destroyed`)

[Devin Rousso]

Comment on attachment 419761 [details]
Patch

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

r=me, nice work!  a few remaining comments to address before landing :)

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:709
> +/* Heading for list of grid nodes */
> +localizedStrings["Grid Overlays @ Layout Sidebar Section Header"] = "Grid Overlays";

NIT: seeing as how the section is already called "Grid" and our documentation uses the term "Page Overlay" <https://webkit.org/web-inspector/page-overlay/>, maybe this should be "Page Overlays @ Layout Sidebar Section Header"?

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:430
> +                node.markDestroyed();

I wonder if this should really be `this._unbind(node)` so that any descendants of the children are also marked?

This might need some testing (preferably LayoutTests, but at least some manual testing).

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:45
> +

Style: extra whitespace (we normally don't add a newline if the assertion is right before an early-return and has the same condition as the following `if`)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:47
> +        if (domNode.destroyed)
> +            return;

NIT: While it is a good defensive practice, since you're already asserting that `!domNode.destroyed`, it's somewhat redundant to also early-return here (especially since the only "error" that can arise from using a destroyed `WI.DOMNode` is that the backend `DOM` agent can't find the node, which won't cause any problems with any of this code since you don't have a callback/`.then` when invoking the command).  We typically use `console.assert` (which are stripped from production builds BTW) as a engineering/developer tool for us to find problems as we build and fix them, but only if those problems won't cause other issues elsewhere.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:76
> +

ditto (:+45)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:78
> +        if (domNode.destroyed)
> +            return;

ditto (:+46)

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:159
> +        this.layoutContextType = payload.layoutContextType;

You should probably add a `|| null` here (and probably in `WI.DOMManager.prototype.nodeLayoutContextTypeChanged`) since older remote inspectors won't provide a `layoutContextType` in the payload and `null !== undefined`.
```
    this.layoutContextType = payload.layoutContextType || null;
```

Alternatively, you could adjust `WI.DOMNode.prototype.set layoutContextType` to also `xor`
```
    if (layoutContextType === this._layoutContextType || !xor(layoutContextType, this._layoutContextType))
```

I'd recommend the first approach though as that's usually what we do for other things like this :)

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:266
> +        this.layoutContextType = null;

please see comment #27

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:982
> +            node.markDestroyed();

ditto (WI.DOMManager:+430)

[Razvan Caliman]

Comment on attachment 419761 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:430
>> +                node.markDestroyed();
> 
> I wonder if this should really be `this._unbind(node)` so that any descendants of the children are also marked?
> 
> This might need some testing (preferably LayoutTests, but at least some manual testing).

Turns out we don't need to call anything there after all. 
If we do, we hit assertions that the node has been destroyed already. Here's why:

`DOMManager._childNodeRemoved()` is called by `DOMObserver.childNodeRemoved()` when a node is removed from the DOM.
`DOMManager._childNodeRemoved()` calls `DOMManager._unbind()` which itself calls `DOMNode.markDestroyed()` which ultimately flips the `layoutContextType` flag.

The same applies to nested child nodes, `DOMManager._childNodeRemoved()` is called for each one.

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:47
>> +            return;
> 
> NIT: While it is a good defensive practice, since you're already asserting that `!domNode.destroyed`, it's somewhat redundant to also early-return here (especially since the only "error" that can arise from using a destroyed `WI.DOMNode` is that the backend `DOM` agent can't find the node, which won't cause any problems with any of this code since you don't have a callback/`.then` when invoking the command).  We typically use `console.assert` (which are stripped from production builds BTW) as a engineering/developer tool for us to find problems as we build and fix them, but only if those problems won't cause other issues elsewhere.

I'd prefer to keep this early return here, even if potentially redundant, for clarity and to prevent any case where the backend fails silently to show the overlay for the destroyed DOMNode, but the `WI.OverlayManager.Event.GridOverlayShown` event still gets dispatched to listeners oblivious of the error.

>> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:159
>> +        this.layoutContextType = payload.layoutContextType;
> 
> You should probably add a `|| null` here (and probably in `WI.DOMManager.prototype.nodeLayoutContextTypeChanged`) since older remote inspectors won't provide a `layoutContextType` in the payload and `null !== undefined`.
> ```
>     this.layoutContextType = payload.layoutContextType || null;
> ```
> 
> Alternatively, you could adjust `WI.DOMNode.prototype.set layoutContextType` to also `xor`
> ```
>     if (layoutContextType === this._layoutContextType || !xor(layoutContextType, this._layoutContextType))
> ```
> 
> I'd recommend the first approach though as that's usually what we do for other things like this :)

Good catch. Forgot to think about backwards compatibility here.

>> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:982
>> +            node.markDestroyed();
> 
> ditto (WI.DOMManager:+430)

No need to do anything here. See comment at WI.DOMManager:+430

[Razvan Caliman]

Created attachment 419841 [details]
Patch

[Razvan Caliman]

(In reply to Razvan Caliman from comment #30)
> Created attachment 419841 [details]
> Patch

Carrying over R+ from previous patch.

[Razvan Caliman]

Created attachment 419842 [details]
[Image] Failed assertions

`DOMNode.destroyed` assertions failed when trying to call `DOMManager._unbind(node)` in `DOMManager._setChildNodes()` and `DOMNode._removeChild()` because the node was already marked as destroyed by separate call to `DOMManager._unbind(node)` from `DOMManager._childNodeRemoved()`

[Devin Rousso]

Comment on attachment 419841 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:159
> +        this.layoutContextType = payload.layoutContextType || null;

thinking about this a bit more, it may actually be simpler to move the `|| null` inside `set layoutContextType` so that it also handles the case where `CSS.layoutContextTypeChanged` does not provide a new `layoutContextType` which would result in `WI.DOMManager.prototype.nodeLayoutContextTypeChanged` being given `undefined` instead of `null`.
```
    set layoutContextType(layoutContextType)
    {
        layoutContextType ||= null;
        if (layoutContextType === this._layoutContextType)
            return;

        this._layoutContextType = layoutContextType;
        this.dispatchEventToListeners(WI.DOMNode.Event.LayoutContextTypeChanged);
    }
```

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:267
> +        this.layoutContextType = null;

Style: i'd add a newline before this

[Razvan Caliman]

Created attachment 419861 [details]
Patch

[Razvan Caliman]

Carry over R+ from comment #28
The latest patch addresses final comments and fixes a layout test.

[BJ Burg]

Comment on attachment 419861 [details]
Patch

The revised patch passes WK2 Tests EWS, setting cq+ to move forward.

[Devin Rousso]

Comment on attachment 419861 [details]
Patch

this is missing a ChangeLog entry for the LayoutTests changes :(

[Razvan Caliman]

Created attachment 419867 [details]
Patch

[EWS]

Committed r272670: <https://commits.webkit.org/r272670>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419867 [details].