Bug 236533

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

<rdar://problem/89173059>

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

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

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 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 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).

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].