WebUI: Replace getElements & getChildren #22220
Conversation
skomerko
changed the title
skomerko
force-pushed
the
webui-mootools-getelements
branch
from
aa56054 to
7528e52
Compare
|
@skomerko |
| @@ -490,7 +490,8 @@ window.addEventListener("DOMContentLoaded", () => { | |||
| const categoryList = document.getElementById("categoryFilterList"); | |||
| if (!categoryList) | |||
| return; | |||
| categoryList.getChildren().each(c => c.destroy()); | |||
|
|
|||
| [...categoryList.children].forEach((el) => { el.destroy(); }); | |||
There was a problem hiding this comment.
Hmm, what exactly .destroy() does? That's not a native JS method (native has .remove() btw).
Also, if the only purpose of this loop is to just clear content of categoryList, simply setting categoryList.innerHTML = "" is wastly easier and more performant way.
Same applies for the rest usages of that construction.
There was a problem hiding this comment.
It removes event listeners, attributes etc. from children, self and detaches node it was called on from parent. There is also this PR #19969 (comment) so I decided to be careful and not touch it for now because I wasn't 100% sure if it's ok to do so.
There was a problem hiding this comment.
Interesting. Event listeners should be removed along with elements, afaik. GC wipes everything not referenced inside the DOM tree or code.
https://stackoverflow.com/questions/12528049/if-a-dom-element-is-removed-are-its-listeners-also-removed-from-memory
So there should be a problematic point elsewhere.
There was a problem hiding this comment.
Indeed. I actually found a memory leak that could explain it but wasn't 100% sure. Each context menu keeps its targets in an array and I noticed it's never cleaned up... I got a fix ready but haven't submitted it yet: master...skomerko:qBittorrent:webui-context-menu-leak
I'm not sure if it's the best way to fix it but it def works.
There was a problem hiding this comment.
Oh no. May I ask why this list/set is even kept around? It doesn't seem to be used anywhere.
There was a problem hiding this comment.
No idea, it was added like ~16 years ago :D So this memory leak probably been here since forever. But yeah it is completely unnecessary to keep references to target elements and not do anything with them. WeakSet would prevent duplicate listeners from being added to targets due to programmer error (e.g. running searchAndAddTargets n times, rather unlikey to happen but still..).
There was a problem hiding this comment.
WeakSet would prevent duplicate listeners from being added to targets due to programmer error
Why not just add some property for elements to track that? Like
addTarget(t) {
if (t.hasEventListeners)
return;
// prevent long press from selecting this text
t.style.userSelect = "none";
t.hasEventListeners = true;
this.setupEventListeners(t);
}There was a problem hiding this comment.
Sure, that's even better (I guess I was just looking for an excuse to finally use WeakSet some more:P). Want to make a PR?
There was a problem hiding this comment.
I guess I was just looking for an excuse to finally use WeakSet some more:P
WeakMap/WeakSet usage is almost never justified. And also slow in comparison with regular Map/Set or object properties.
Want to make a PR?
Nah, I'm too busy rn, and you are already onto it.
This PR further reduces Mootools usage.