Code hints leak memory on each invocation

[peterflynn]

Code hints currently leak memory because of a bug in PopUpManager: removePopUp() doesn't distinguish between explicit calls to that method from outside, vs. automatic calls from PopUpManager's internal Esc key handler. Net result: unless you created the popup with autoAddRemove == true (which code hints don't), it will never get removed from the _popUps array. So the entire DOM subtree is retained in memory forever.

[pthiess]

Reviewed. If false shouldn't be an option we should consider to remove.

[pthiess]

Context menu / add menu pass false, so we may need to fix it.

[redmunds]

FBNC bck to @peterflynn

[peterflynn]

I can confirm we no longer have any scenarios in Brackets core that leak memory. However, I think this API is still very confusing and easy to misuse (including creating new memory leaks later). For example:

• If you pass false to addPopUp(), it's still impossible to free the memory later. Passing false is only appropriate for permanent parts of the UI that are never dynamically created (such as the menu bar... though that stops being true as soon as we add a removeMenu() API).
• removePopUp() is essentially only useful if you've passed true to addPopUp(). It's easy to mistakenly believe it'd do something useful (and the docs don't say otherwise). It seems unnecessary to make it a public API anyway, since in the 'true' case it gets automatically called for you.

We could replace the autoRemove flag with something like a 'neverRemoved' or 'permanent' flag to make it more clear it's only appropriate for UI that will never go away. But since I think such UI will be very rare, I think doing a non-leaky version of the old semantic would be better. That is, the two boolean choices would be automatic removal / manual removal (vs. today where the choices are automatic removal / impossible to ever remove).

[peterflynn]

Oops, meant to reopen & retag as low-priority code cleanup...

[njx]

Could we consolidate these issues into #1381? That's where I'm tracking various issues relating to how we deal with popups and PopUpManager.

[joelrbrandt]

@njx @peterflynn I don't know if this is related, but i just noticed that TagHints.prototype.getQueryInfo gets called three times in rapid succession when I type a "<" in an HTML file. (I.e. when I start a new tag and have an empty query.)