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

RESOLVED FIXED

Status

SeaMonkey
UI Design
--
minor
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: ewong)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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.

Updated

7 years ago
See Also: → bug 668972
(Assignee)

Comment 1

6 years ago
Created attachment 606233 [details] [diff] [review]
Clear the popup blocker's list of popup items everytime the menu is hidden. (v1)
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #606233 - Flags: review?(neil)
(Reporter)

Comment 2

6 years ago
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.)
Attachment #606233 - Flags: review?(neil) → review-
(Assignee)

Comment 3

6 years ago
Created attachment 606434 [details] [diff] [review]
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.
Attachment #606233 - Attachment is obsolete: true
Attachment #606434 - Flags: review?(neil)

Comment 4

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

Comment 5

6 years ago
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)".
(Assignee)

Comment 6

6 years ago
Created attachment 607517 [details] [diff] [review]
Clear the popup blocker's list of popup items everytime the menu is hidden. (v3)
Attachment #606434 - Attachment is obsolete: true
Attachment #606434 - Flags: review?(neil)
Attachment #607517 - Flags: review?(neil)
(Reporter)

Comment 7

6 years ago
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)
(Assignee)

Comment 8

6 years ago
Created attachment 609633 [details] [diff] [review]
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.
Attachment #607517 - Attachment is obsolete: true
Attachment #607517 - Flags: review?(neil)
Attachment #609633 - Flags: review?(neil)
(Reporter)

Updated

6 years ago
Attachment #609633 - Flags: review?(neil) → review+
(Assignee)

Comment 9

6 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/d6cf9c7638ee
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.