Bug 221919 – Web Inspector: Grids with overlays that disappear and reappear remain checked in Layout pane, toggling hits assertion

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

**Steps to repro:**
1. Navigate to https://labs.jensimmons.com/2017/03-008.html.
2. Enable the grid overlay for `main` in the Layout sidebar.
3. Resize page width until the content reflows to no longer use grid.
4. Resize page width back to original (or close to it).
5. Click the already-checked main checkbox.

**Expected Result:**
The checkbox would no longer be checked OR the overlay would be reenabled when it becomes a grid again.

**Actual Result:**
The checkbox is still checked without a grid overlay visible and unchecking the box results in:
No grid overlay exists for the node, so cannot clear. (at Connection.js:​162:​29)​
    _dispatchResponseToPromise @ Connection.js:​162:​29
    _dispatchResponse @ Connection.js:​124:​44
    dispatch @ Connection.js:​77:​35
    dispatchMessageFromTarget @ TargetManager.js:​176:​39
    dispatchMessageFromTarget @ TargetObserver.js:​47:​51
    _dispatchEvent @ Connection.js:​210:​26
    dispatch @ Connection.js:​79:​32
    dispatch @ InspectorBackend.js:​232:​52
    ?​ @ MessageDispatcher.js:​42:​34

[Radar WebKit Bug Importer]

<rdar://problem/74363006>

[Razvan Caliman]

Created attachment 420462 [details]
Patch

[Devin Rousso]

Comment on attachment 420462 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:34
> +        WI.DOMNode.addEventListener(WI.DOMNode.Event.LayoutContextTypeChanged, this._handleLayoutContextTypeChanged, this);

Rather than listening for every single layout context type change, can you just add this event listener inside `showGridOverlay` (and remove it in `hideGridOverlay`)?  This way we don't have to run code for _every_ layout context type change (which is almost guaranteed going to be a far larger amount than just the handful (if even that many) of active overlays).  This would also let you avoid having to check `this._gridOverlayForNodeMap.has(domNode)` in the event handler.

[Razvan Caliman]

Created attachment 420503 [details]
Patch

[Razvan Caliman]

Comment on attachment 420462 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:34
>> +        WI.DOMNode.addEventListener(WI.DOMNode.Event.LayoutContextTypeChanged, this._handleLayoutContextTypeChanged, this);
> 
> Rather than listening for every single layout context type change, can you just add this event listener inside `showGridOverlay` (and remove it in `hideGridOverlay`)?  This way we don't have to run code for _every_ layout context type change (which is almost guaranteed going to be a far larger amount than just the handful (if even that many) of active overlays).  This would also let you avoid having to check `this._gridOverlayForNodeMap.has(domNode)` in the event handler.

That's a good idea! Uploaded a new patch implementing this.

[Devin Rousso]

Comment on attachment 420503 [details]
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));
```

[Razvan Caliman]

Comment on attachment 420503 [details]
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));
> ```

> You also probably should remove it from `_gridOverlayForNodeMap`.

That's what `this._gridOverlayForNodeMap.take(domNode)` is for.

[Devin Rousso]

Comment on attachment 420503 [details]
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));
>> ```
> 
> 

Oh I totally missed that!  Very clever move :)

You might consider pulling that out into a variable tho just so that it's a bit more visible :P

[Razvan Caliman]

Created attachment 420520 [details]
Patch

Carry over R+ from previous

[Razvan Caliman]

(In reply to Razvan Caliman from comment #9)
> Created attachment 420520 [details]
> Patch
> 
> Carry over R+ from previous

Scratch that. No R+ to carry over. I got confused.

[Devin Rousso]

Comment on attachment 420520 [details]
Patch

(In reply to Razvan Caliman from comment #10)
> (In reply to Razvan Caliman from comment #9)
> > Created attachment 420520 [details]
> > Patch
> > 
> > Carry over R+ from previous
> 
> Scratch that. No R+ to carry over. I got confused.
I actually did r+ the previous patch, but I forgot to write "r=me".  You can click the "History" link in the top-right next to "Modified:" and see for yourself :)

[Razvan Caliman]

(In reply to Devin Rousso from comment #11)
> Comment on attachment 420520 [details]
> Patch
> 
> (In reply to Razvan Caliman from comment #10)
> > (In reply to Razvan Caliman from comment #9)
> > > Created attachment 420520 [details]
> > > Patch
> > > 
> > > Carry over R+ from previous
> > 
> > Scratch that. No R+ to carry over. I got confused.
> I actually did r+ the previous patch, but I forgot to write "r=me".  You can
> click the "History" link in the top-right next to "Modified:" and see for
> yourself :)

Ha! Switching quickly between working on two patches. I knew I saw some R+ somewhere :)
Thanks for the History tip!

[EWS]

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

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