676220 - Popup blocker menu should be cleared more eagerly to avoid holding on to window objects

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

This is the SeaMonkey version of bug 668972.

The popup blocker menu maintains a list of recently blocked popups. This list should be cleared so that it doesn't hang around until the browser is closed.

Comment 1

[Edmund Wong (:ewong)]

Attachment: Clear the popup blocker's list of popup items everytime the menu is hidden. (v1)

Comment 2

[[e-mail]]

Comment on attachment 606233 [details] [diff] [review]
Clear the popup blocker's list of popup items everytime the menu is hidden. (v1)

We actually have three popup notification menus:
a. The notificationbar's popup preferences button popup
b. The browser's popup statusbarpanel context menu
c. The browser's tools > popup manager submenu
Note that the former uses popupNotificationMenuShowing but the others use popupBlockerMenuShowing (the latter indirectly via CheckForVisibility).

>+function PopUpHide(aEvent)
Nit: method is intended to undo the effect of createShowPopupsMenu, maybe a better name could be used. Also this belongs above the above method.

>+  let item = aEvent.target.lastChild;
>+  while (item && item.getAttribute("id") != "popupNotificationMenuSeparator")
>+  {
>+    let next = item.previousSibling;
>+    item.parentNode.removeChild(item);
>+    item = next;
>+  }
Nit: You didn't remove the existing code in createShowPopupsMenu that also tries to clear down the menu. (In fact, you could just move that code here.)

Comment 3

[Edmund Wong (:ewong)]

Attachment: Clear the popup blocker's list of popup items everytime the menu is hidden. (v2)

>We actually have three popup notification menus:
>a. The notificationbar's popup preferences button popup
>b. The browser's popup statusbarpanel context menu
>c. The browser's tools > popup manager submenu
>Note that the former uses popupNotificationMenuShowing but the others use >popupBlockerMenuShowing (the latter indirectly via CheckForVisibility).

I believe this patch covers these.

>>+function PopUpHide(aEvent)
>Nit: method is intended to undo the effect of createShowPopupsMenu, maybe a >better name could be used. Also this belongs above the above method.

Changed to RemovePopupMenu.
>+  let item = aEvent.target.lastChild;
>+  while (item && item.getAttribute("id") != "popupNotificationMenuSeparator")
>+  {
>+    let next = item.previousSibling;
>+    item.parentNode.removeChild(item);
>+    item = next;
>+  }
>Nit: You didn't remove the existing code in createShowPopupsMenu that also >tries to clear down the menu. (In fact, you could just move that code here.)

Fixed.

Comment 4

[Philip Chee]

> +function RemovePopupsMenu(aEvent)
.....
> +               onpopuphiding="RemovePopupsMenu(event, 'popupNotificationMenu');"/>
You're calling RemovePopupsMenu with two parameters, but the function is expecting only one.

Comment 5

[[e-mail]]

Comment on attachment 606434 [details] [diff] [review]
Clear the popup blocker's list of popup items everytime the menu is hidden. (v2)

>+function RemovePopupsMenu(aEvent)
Don't actually need this because we automatically get the utilityOverlay one.

>+               onpopuphiding="RemovePopupsMenu(event, 'popupNotificationMenu');"/>
Please change these to pass this instead of event (and update the function).

>+function RemovePopupsMenu(aEvent)
Please move this before createShowPopupsMenu.

A slightly better name for this might be "RemovePopupsMenuitems(parent)".

Comment 6

[Edmund Wong (:ewong)]

Attachment: Clear the popup blocker's list of popup items everytime the menu is hidden. (v3)

Comment 7

[[e-mail]]

Comment on attachment 607517 [details] [diff] [review]
Clear the popup blocker's list of popup items everytime the menu is hidden. (v3)

>+function RemovePopupsItemMenu(aThis)
Nits:
1. Not happy with "aThis", try changing it to parent?
   (I know it doesn't begin with an "a", but you'll see why in the diff!)
2. Function name still doesn't look quite right. Try RemovePopupsItems(parent)

Comment 8

[Edmund Wong (:ewong)]

Attachment: Clear the popup blocker's list of popup items everytime the menu is hidden. (v4)

>1. Not happy with "aThis", try changing it to parent?

Fixed.

>2. Function name still doesn't look quite right. Try RemovePopupsItems(parent)

Fixed.

Comment 9

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/d6cf9c7638ee