Bug 236533 – Web Inspector: merge WI.OverlayManager into WI.DOMNode

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

There's really nothing substantial about `WI.OverlayManager` that requires it to exist. All of the methods, members, etc. can be moved to `WI.DOMNode`, resulting in clearer calling code (since callers currently have to provide a `WI.DOMNode` to `WI.OverlayManager` methods) and fewer collections (e.g. it's no longer necessary to have a "list of nodes showing a grid overlay").

[Devin Rousso]

Created attachment 451759 [details]
[fast-cq] Patch

[Radar WebKit Bug Importer]

<rdar://problem/89173059>

[Devin Rousso]

Created attachment 452787 [details]
[fast-cq] Patch

[Jonathan Bedard]

3 failures in https://ews-build.webkit.org/#/builders/70/builds/973 are known, stopped the build before retries.

[Patrick Angle]

Comment on attachment 452787 [details]
[fast-cq] Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/GridOverlayConfigurationDiagnosticEventRecorder.js:35
> +        this._nodesShowingGridLayoutOverlay = new Set;

Is there a way we can do this by just keeping a balanced count instead of keeping the nodes around, or do the events that are fired not guarantee any sort of balanced calls? If not, can we add a listener for node removal (or maybe just a new event called from WI.DOMManager.prototype.willDestroyDOMNode) to also remove it from this set?

[Devin Rousso]

Comment on attachment 452787 [details]
[fast-cq] Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/GridOverlayConfigurationDiagnosticEventRecorder.js:35
>> +        this._nodesShowingGridLayoutOverlay = new Set;
> 
> Is there a way we can do this by just keeping a balanced count instead of keeping the nodes around, or do the events that are fired not guarantee any sort of balanced calls? If not, can we add a listener for node removal (or maybe just a new event called from WI.DOMManager.prototype.willDestroyDOMNode) to also remove it from this set?

Maybe?  I believe there is some complexity in that when the `layoutContextType` changes the overlay is automatically hidden in the backend, but the frontend should dispatch `WI.DOMNode.Event.LayoutOverlayHidden` just in case.

I've actually been wanting to introduce a `IterableWeakSet` (of my creation) so maybe this is a good "excuse" :)

[Devin Rousso]

Comment on attachment 452787 [details]
[fast-cq] Patch

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

>>> Source/WebInspectorUI/UserInterface/Controllers/GridOverlayConfigurationDiagnosticEventRecorder.js:35
>>> +        this._nodesShowingGridLayoutOverlay = new Set;
>> 
>> Is there a way we can do this by just keeping a balanced count instead of keeping the nodes around, or do the events that are fired not guarantee any sort of balanced calls? If not, can we add a listener for node removal (or maybe just a new event called from WI.DOMManager.prototype.willDestroyDOMNode) to also remove it from this set?
> 
> Maybe?  I believe there is some complexity in that when the `layoutContextType` changes the overlay is automatically hidden in the backend, but the frontend should dispatch `WI.DOMNode.Event.LayoutOverlayHidden` just in case.
> 
> I've actually been wanting to introduce a `IterableWeakSet` (of my creation) so maybe this is a good "excuse" :)

It's also worth mentioning that the old code used `WI.OverlayManager.prototype.hasVisibleGridOverlays`, which consulted the private `_overlayForNodeMap` (which was a `Map`), so there should be no difference in what is kept alive before vs after this change.

I'm currently working on writing some tests for the `IterableWeakSet` I've created, but it's difficult due to the fact that there's no way for us to ensure that a given object is GCd, and therefore the any tests for any `Weak*` would be potentially flaky.

Based on my first comment, I'm gonna land this and try to get `IterableWeakSet` working separately instead of delaying this change (not to mention `IterableWeakSet` is kinda only tangentially related to this change, so adding it as part of this could be seen as odd).

[EWS]

Committed r290435 (247738@main): <https://commits.webkit.org/247738@main>

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