669291 - Fold permissionsNavigatorOverlay.xul into navigator code

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Ian Neal]

Attachment: Fold in permissionsNavigatorOverlay.xul

At the moment, for historical reasons, we have some permissions overlays such as permissionsNavigatorOverlay.xul. This could be folded into the navigator code and hopefully help tidy up the code at the same time.

This patch:
* Moves xul code into navigatorOverlay.xul file.
* Moves js code into navigator.js file with some simplification.
* Switches js code to use Services.jsm where applicable.
* Removes the old xul/dtd files and jar.mn entries.

Potentially having perm set to -1 is overkill and default could be the same as "default" and not do the test for perm == -1, this would simplify things more.

Comment 1

[[e-mail]]

Comment on attachment 543917 [details] [diff] [review]
Fold in permissionsNavigatorOverlay.xul

>+  var perm = -1;
Not sure what the point of this is, you always pass in a valid type.

>+  switch (action) {
>+    case "allow":
>+      perm = Services.perms.ALLOW_ACTION;
>+      break;
>+    case "session":
>+      perm = Components.interfaces.nsICookiePermission.ACCESS_SESSION;
>+      break;
>+    case "default":
>+      perm = Services.perms.UNKNOWN_ACTION;
>+      break;
>+    case "block":
>+      perm = Services.perms.DENY_ACTION;
>+      break;
These could be stored in a global lookup table i.e.
var gActions = {
  "allow": Components.interfaces.nsIPermissionManager.ALLOW_ACTION,
  "session": // etc.

Or maybe you could look them up directly:
Components.interfaces.nsICookiePermission["ACCESS_" + action.toUpperCase()]

>+  setRadioButton("cookie_default", uri);
>+  setRadioButton("cookie_allow", uri);
>+  setRadioButton("cookie_session", uri);
>+  setRadioButton("cookie_block", uri);
[Bah, this ends up testing the cookie permission four times...]

>+  Services.perms.add(uri, type, perm);
I don't think this works for UNKNOWN_ACTION. At least, after I tried to select the default action, all of the menuitems stopped working, so marking r-.

>+                      oncommand="CookieImageAction(this);"/> 
Nit: trailing space.
[If you could work around the cookie manager option, you could replace this with event.target and put it on the parent menu to work for all menuitems.]

>+                      checked="true"
[Hardly worth checking one, since you're going to update it anyway.]

Comment 2

[Ian Neal]

Attachment: Fold in permissionsNavigatorOverlay.xul done by name [Checked in: Comment 4]

Changes since last version:
* Only test cookie and image permissions the once when setting checked attributes.
* Use action.toUpperCase() to get permission from menuitem's id.
* Use event.target on menu rather than this on each menuitem.
* Removed unneeded setting of checked in xul.
* Removed trailing spaces.
* Tested that switching between default and non-default actions does not break.

Comment 3

[[e-mail]]

Comment on attachment 545104 [details] [diff] [review]
Fold in permissionsNavigatorOverlay.xul done by name [Checked in: Comment 4]

>+  var items = document.getElementsByAttribute("name", aType);
[Might be better to start the search from the popup, if you can.]

>+  Services.perms.add(uri, type, perm);
Nice, this does in fact work for UNKNOWN_ACTION. (Although there's an edge case as the default is to inherit from the parent domain, so although you can set to default you then get shown the new inherited value.)

>+<!ENTITY % navigatorOverlayDTD SYSTEM "chrome://navigator/locale/navigatorOverlay.dtd">
>+%navigatorOverlayDTD;
[I wonder whether we should start moving stuff here from navigator.dtd]

>+      <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, in a followup if you prefer.

>+                         else toDataManager('|permissions');">
[I wonder whether these should use the hostport as well.]

Comment 4

[Ian Neal]

Comment on attachment 545104 [details] [diff] [review]
Fold in permissionsNavigatorOverlay.xul done by name [Checked in: Comment 4]

http://hg.mozilla.org/comm-central/rev/0e65da451a67

Comment 5

[Ian Neal]

(In reply to comment #3)
> Comment on attachment 545104 [details] [diff] [review] [review]
> Fold in permissionsNavigatorOverlay.xul done by name [Checked in: Comment 4]
> 
> >+  var items = document.getElementsByAttribute("name", aType);
> [Might be better to start the search from the popup, if you can.]
Done for checkin.
> 
> >+  Services.perms.add(uri, type, perm);
> Nice, this does in fact work for UNKNOWN_ACTION. (Although there's an edge
> case as the default is to inherit from the parent domain, so although you
> can set to default you then get shown the new inherited value.)
If you have ideas that you can put into an enhancement bug, please do.
> 
> >+<!ENTITY % navigatorOverlayDTD SYSTEM "chrome://navigator/locale/navigatorOverlay.dtd">
> >+%navigatorOverlayDTD;
> [I wonder whether we should start moving stuff here from navigator.dtd]
Spun off into bug 670750
> 
> >+      <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, in a followup if you prefer.
Spun off into bug 670749
> 
> >+                         else toDataManager('|permissions');">
> [I wonder whether these should use the hostport as well.]
Checking with KaiRo if this would make sense to Data Manager

Comment 6

[[e-mail]]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 545104 [details] [diff] [review])
> > >+  Services.perms.add(uri, type, perm);
> > Nice, this does in fact work for UNKNOWN_ACTION. (Although there's an edge
> > case as the default is to inherit from the parent domain, so although you
> > can set to default you then get shown the new inherited value.)
> If you have ideas that you can put into an enhancement bug, please do.
Well, possibly not enough for a bug... the way the permission manager works, is that if you, say, block images from memebase.com then that propagates to all subsites such as comixed.memebase.com and the image blocker menu will show that images are blocked. You can of course override that and allow images on comixed.memebase.com but if you then try to set default what happens is that it goes back to propagating from memebase.com so it actually shows as blocked, which isn't what you just selected. My best idea was to only enable the set default option if the permission has been set directly and not propagated. That way you could see that the site was set to default (because the option was disabled) but the effective state is still shown as blocked. Note that if we do this we may want to be able to set a particular permission directly even though it has already propagated as the effective state.

Comment 7

[[e-mail]]

Now that I've written that essay, I guess that proves that I need a new bug!