[webkit-reviews] review granted: [Bug 221919] Web Inspector: Grids with overlays that disappear and reappear remain checked in Layout pane, toggling hits assertion : [Attachment 420503] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Razvan Caliman
<rcaliman at apple.com>'s request for review:
Bug 221919: Web Inspector: Grids with overlays that disappear and reappear
remain checked in Layout pane, toggling hits assertion
https://bugs.webkit.org/show_bug.cgi?id=221919

Attachment 420503: Patch

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




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

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

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:102
> +	   if (domNode.layoutContextType !== WI.DOMNode.LayoutContextType.Grid)
{

Style: we often prefer early-returns to avoid extra indentation

Also, I think you should be able to assume that the new `layoutContextType` is
not `WI.DOMNode.LayoutContextType.Grid` seeing as how the only way that
`showGridOverlay` can get called is if the `domNode` is already a
`WI.DOMNode.LayoutContextType.Grid` context.  We should probably add
`console.assert(domNode.layoutContextType ===
WI.DOMNode.LayoutContextType.Grid, domNode.layoutContextType);` inside
`showGridOverlay` and `hideGridOverlay`.

You also probably should remove it from `_gridOverlayForNodeMap`.
```
    console.assert(domNode.layoutContextType !==
WI.DOMNode.LayoutContextType.Grid, domNode);
    this._gridOverlayForNodeMap.delete(domNode);
    domNode.removeEventListener(WI.DOMNode.Event.LayoutContextTypeChanged,
this._handleLayoutContextTypeChanged, this);
    this.dispatchEventToListeners(WI.OverlayManager.Event.GridOverlayHidden,
this._gridOverlayForNodeMap.take(domNode));
```