670749 - Have individual methods for cookie/image/popup menus in navigatorOverlay

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Ian Neal]

From bug 669291 comment 3

>+      <menupopup id="taskPopup" onpopupshowing="CheckForVisibility();">
It just occurs to me that this gets called each time the menu or submenu is opened... probably worth removing this in favour of individual methods for each of the permission submenus

Comment 1

[Ian Neal]

Attachment: Create separate functions for popup and non-popup permission menus

This patch:
* Moves the cookie/image code into separate function and calls from relevant menu with a type argument.
* Passes event argument to CheckForVisibility so that the non-mac code can be moved from the onpopupshowing to the function.
* Moves the caller to CheckForVisibility from task menupopup to popup manager menu.

Comment 2

[[e-mail]]

Comment on attachment 545353 [details] [diff] [review]
Create separate functions for popup and non-popup permission menus

>+function CheckPermissionsMenu(aType)
> {
>+  // Determine current state and check/uncheck the appropriate menu items.
>+  setRadioButtons(getBrowser().currentURI, aType);
Can you not move the call to get the current URI into setRadioButtons? (You may subsequently want to rename setRadioButtons to CheckPermissionsMenu).

>   var items = document.getElementById("menu_" + aType + "Manager")
>                       .getElementsByAttribute("name", aType);
You could get the target from the popupshowing handler event.

>         <menu id="menu_cookieManager"
>               label="&cookieCookieManager.label;"
>               accesskey="&cookieCookieManager.accesskey;"
>+              onpopupshowing="CheckPermissionsMenu('cookie');"
>               oncommand="if (event.target.id) CookieImageAction(event.target);
>                          else toDataManager('|cookies');">
>           <menupopup>
Nit: Please move the popupshowing event handler to the popup.

Comment 3

[Ian Neal]

Attachment: Moved callers to separate functions [Checked in: Comment 5]

Changes since last version:
* As suggested moved currentURI to setRadioButtons and renamed function to CheckPermissionsMenu.
* Passes this as an argument to CheckPermissionsMenu.
* Moved function callers to menupopup from menu element.

Comment 4

[[e-mail]]

Comment on attachment 545401 [details] [diff] [review]
Moved callers to separate functions [Checked in: Comment 5]

>+  if (!/Mac/.test(navigator.platform))
>+    return popupBlockerMenuShowing(aEvent);
> }
[The return is and has always been unnecessary, and is in fact a JS Strict warning, but I don't know why neither version gets reported.]

>+  var currentPerm = Services.perms.testPermission(getBrowser().currentURI,
>+                                                  aType);
[Hardly worth wrapping this for two characters. If you're really that worried you could try shortening the currentPerm variable.]

Comment 5

[Ian Neal]

Comment on attachment 545401 [details] [diff] [review]
Moved callers to separate functions [Checked in: Comment 5]

http://hg.mozilla.org/comm-central/rev/7c0c838f516d