Bug 236533 - Web Inspector: merge WI.OverlayManager into WI.DOMNode
Summary: Web Inspector: merge WI.OverlayManager into WI.DOMNode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-11 17:00 PST by Devin Rousso
Modified: 2022-02-24 09:24 PST (History)
6 users (show)

See Also:


Attachments
[fast-cq] Patch (52.36 KB, patch)
2022-02-11 17:02 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[fast-cq] Patch (51.44 KB, patch)
2022-02-21 15:28 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2022-02-11 17:00:25 PST
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").
Comment 1 Devin Rousso 2022-02-11 17:02:42 PST
Created attachment 451759 [details]
[fast-cq] Patch
Comment 2 Radar WebKit Bug Importer 2022-02-18 17:01:20 PST
<rdar://problem/89173059>
Comment 3 Devin Rousso 2022-02-21 15:28:09 PST
Created attachment 452787 [details]
[fast-cq] Patch
Comment 4 Jonathan Bedard 2022-02-22 08:21:08 PST
3 failures in https://ews-build.webkit.org/#/builders/70/builds/973 are known, stopped the build before retries.
Comment 5 Patrick Angle 2022-02-23 17:01:54 PST
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?
Comment 6 Devin Rousso 2022-02-23 17:43:41 PST
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" :)
Comment 7 Devin Rousso 2022-02-24 08:40:24 PST
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).
Comment 8 EWS 2022-02-24 08:44:06 PST
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].