307928 - if Options - Content - Allow websites to install software - is disabled : EM/TM should not have update button/context menu

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050910
Firefox/1.4 ID:2005091004

repro:
1. Open FF
2. Go to Tools - Options - Content and UNcheck "Allow websited to install software"
3. Close/Reopen FF
4. Go to EM or TM and notice the update button/context menu is there (it doesn't
work though)

expected:
buttons/contextmenu for updates to be either removed or grayed out

Comment 1

[Weston Carloss]

Another option is the bar that says, "Software installation is currently
disabled. Click Edit Options...to enable it and try again" could pop up, just
like it does when you try to install a new extension with software installation
disabled. 

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I wonder about this though... If a person is manually choosing to update their 
extensions through the EM then does that really count as allowing a website to 
install software? It takes concerted effort on the user's part to get to this 
spot (unlike a web page trying to do it without user intervention). I would 
just be annoyed, I think, if I had to go into the prefs every time I wanted to 
manually update my extensions. Anyway, this is kind of the same argument 
that's going on in bug 288054. (I think they're going to change the wording of 
the pref...)

This comment extends my own:
https://bugzilla.mozilla.org/show_bug.cgi?id=288054#c28

Comment 3

[Robert Strong (they/them - no direct email)]

I held off on adding behavior like this a while ago in the hope that a fix for
bug 288054 would land since the current behavior needs to be changed and what
the update buttons due will most likely depend on that fix.

Comment 4

[Weston Carloss]

Thanks for the explanation Robert and Ben.  I'll probably also post this in
288054 bug; here's how I think the update options shoudl be handled.  

The checkbox next to "Allow websites to install software" should be removed,
since, as Daniel said, this is a whitelist already and you shouldn't have to
enable an option for it.  The text should be changed to something like, "Allow
the following websites to install software" or "Only these sites can install
software."  The word software could also be replaced by extensions/themes,
although this might make the options box too wide.  The wording on the "Allowed
sites" button could either be left alone, or changed to a simple "Configure."  I
suppose "configure" might sound intimidating to some users, so that may not be
the best word choice.  

Honestly, I don't know the inner workings of the update system, but I'm going to
make up a procedure by taking a guess at how the system works.  xpinstall.enable
should always be set to false.  If the user tries to install software from a
site on the whitelist, xpinstall should be temporarily enabled so that the
installation can continue, and once this installation is complete, the setting
should disabled again.  I don't know whether install has to be enabled only to
the point of receiving the message "(item) will be installed after restart" or
if it must be enabled until Firefox is restarted.  If the user tries to receive
install something from a site not on the whitelist, the bar should pop up, but
the wording should be changed to, "This site is not currently allowed to install
software.  If you wish to allow it, click here" and have a button that says
"allow site" or something similar.  If the user clicks an update now button or
autoupdate detects a required update, xpinstall should again be temporarily
enabled for as long as it has to be, then disabled.  Another possibility is to
allow updates to work with xpinstall disabled, the way that a build update
currently works, and onyl have make new software installation dependent on the
xpinstall setting.

Sorry for the long post and if a lot of my ideas are unfeasible, but as I said,
I don't know very much about how the xpi or update systems actually work.  I
just wanted to offer a suggestion for how this bug and 288054 could be dealt with.

Comment 5

[Robert Strong (they/them - no direct email)]

Once dveditz gets bug 288054 fixed the behavior here can be flushed out but
until then it is all guess work without much value.

Comment 6

[Daniel Veditz [:dveditz]]

bug 288054 has landed on trunk. This bug could be resolved WFM, or if you're
still concerned about the case where someone has manually set the
xpinstall.enabled pref false then you could add a pref observer and some code to
disable the update button/menuitem -- but I'm not sure it's worth the effort.

Comment 7

[Bradley Chapman (not reading bugmail, still gone but not forever)]

To be honest, I think that disabling the UI would indeed be worth the trouble -
either that or making the UI do something else when updating is disabled.

Even though bug 288054 exchanged the checkbox controlling xpinstall.enabled for
a checkbox controlling xpinstall.whitelist.required, power users that want to
turn off XPI installation altogether using about:config will probably not want
to see the UI acting like it still works.

It doesn't even have to be a pref observer: you could simply do a pref-check on
xpinstall.enabled and app.update.enabled and show something from the prompt
service like this:

 ---------------------------------------------------------
 |                      Firefox                          |
 ---------------------------------------------------------
 |                                                       |
 |  [ICON]  Software Update has been disabled.           |
 |                                                       |
 |                                              [OK]     |
 |-------------------------------------------------------|

(This of course pre-supposes that turning off app.update.enabled doesn't already
 disable the UI in the EM/TM and the Help menu).

Would something like this, written to be as minimal as possible with minor l10n
impact, have any chance of being taken for 1.5?

Comment 8

[Robert Strong (they/them - no direct email)]

I think just disabling the widgets (e.g. by disabling the commands) would
suffice and this would have no l10n impact.

Comment 9

[Bradley Chapman (not reading bugmail, still gone but not forever)]

I'd like to try to make a patch for this, but before I do, I've got a few impl.
questions:

1 - should the pref observer be integrated with one of the global structs like
gExtensionsViewController, or should it be broken out into its own global (i.e.
gExtensionsPrefObserver)?

2 - is it sufficient to disable the cmd_update and cmd_update_all commands?

Comment 10

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Nevermind, I answered both questions myself. Patch coming up.

Comment 11

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Attachment: 307928-updateUI-v0.1.patch

This patch adds a preference observer to the backend EM/TM code which watches
the value of the preference "xpinstall.enabled" and enables/disables the XUL
update command elements inside the extensionCommandSet. It also changes the
Startup() routine in the backend code to check the value of "xpinstall.enabled"
and disable/enable the commands there as well.

The original patch was created against the shipped extensions.js in the
20050929 Firefox nightly build. Since I can't use CVS due to firewalling, the
above patch has been rediffed against the extensions.js in the Firefox 1.5b1
source tarball.

Comment 12

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Comment on attachment 197999 [details] [diff] [review]
307928-updateUI-v0.1.patch

Requesting review from Robert Strong, as per the Toolkit Review Requirements
document.

Comment 13

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Ugh. I just noticed that accessing global-scoped constants from a global struct
seems to throw a wobbly in the JavaScript Console, yet it doesn't break anything
else...

If necessary I'll resubmit a new patch, but I'd prefer comments on the above one
first.

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 197999 [details] [diff] [review]
307928-updateUI-v0.1.patch

> const PREF_GENERAL_SKINS_SELECTEDSKIN       = "general.skins.selectedSkin";
> 
>+const PREF_XPINSTALL_ENABLED                = "xpinstall.enabled";
>+
Remove the empty line separating this from the existing pref constants

>+var gExtensionsPrefObserver = {
>+  // aTopic should be an nsISupports instance, aSubject should be "nsPref:changed"
>+  // and aData should be the name of the pref that was changed
>+  observe: function(aTopic, aSubject, aData)
>+  {
<snip>
The following is a cleaner way to do this.
var gExtensionsPrefObserver = {
  observe: function (aSubject, aTopic, aData)
  {
    if (aTopic == "nsPref:changed") {
      switch(aData) {
	case PREF_XPINSTALL_ENABLED:
	  'do stuff'
      }
    }
  }
};


For the remainder there is already an attribute on gExtensionsView named
'update-check' used to accomplish this same affect during an update. You should
be able to re-use it to simplify the code - just change the name to something
more appropriate.

Comment 15

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #14)
> (From update of attachment 197999 [details] [diff] [review] [edit])
> > const PREF_GENERAL_SKINS_SELECTEDSKIN       = "general.skins.selectedSkin";
> > 
> >+const PREF_XPINSTALL_ENABLED                = "xpinstall.enabled";
> >+
> Remove the empty line separating this from the existing pref constants

Done.

> 
> >+var gExtensionsPrefObserver = {
> >+  // aTopic should be an nsISupports instance, aSubject should be
"nsPref:changed"
> >+  // and aData should be the name of the pref that was changed
> >+  observe: function(aTopic, aSubject, aData)
> >+  {
> <snip>
> The following is a cleaner way to do this.
> var gExtensionsPrefObserver = {
>   observe: function (aSubject, aTopic, aData)
>   {
>     if (aTopic == "nsPref:changed") {
>       switch(aData) {
> 	case PREF_XPINSTALL_ENABLED:
> 	  'do stuff'
>       }
>     }
>   }
> };

Done. I wouldn't mind some feedback on the comments marked XXX, though. I want
to ensure that the issues outlined won't impact the stability of the 1.8 branch.

> 
> 
> For the remainder there is already an attribute on gExtensionsView named
> 'update-check' used to accomplish this same affect during an update. You should
> be able to re-use it to simplify the code - just change the name to something
> more appropriate.
> 

Ah, I hadn't noticed that. I'll try using the 'update-check' attribute and see
if it accomplishes the same effect.

Comment 16

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Comment on attachment 197999 [details] [diff] [review]
307928-updateUI-v0.1.patch

Sorry Rob, but the gExtensionsViewController and/or the buildContextMenu()
function is stripping the 'update-check' attribute whenever I right-click one
of the richlistbox elements. That was why I needed the workaround in
gExtensionsViewController.onCommandUpdate().

Is it really that unacceptable to check for the 'skipupdate' attribute? It
could come in handy for the other available commands within the
extensionCommandSet.

Comment 17

[Robert Strong (they/them - no direct email)]

Please submit a patch where you did it this way... I just tried out the changes
using the existing update-check attribute on gExtensionsView as is used
elsewhere and it WFM without stripping it when right clicking.

I should have time to comment on the XXX's tomorrow.

Comment 18

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Attachment: 307928-updateUI-v0.2.patch

The above patch has the initial round of nits discovered by Rob picked
(hopefully).

Requesting review from Rob once more.

Comment 19

[Bradley Chapman (not reading bugmail, still gone but not forever)]

As an aside, your way does indeed work - I guess my work space was inconsistent
in some way.

Comment 20

[Robert Strong (they/them - no direct email)]

Comment on attachment 198076 [details] [diff] [review]
307928-updateUI-v0.2.patch

This is much closer.

>+  // Install our preference observer on XPINSTALL_PREF_ENABLED. This fixes bug 307928.
>+  // XXX: does the strength of the reference matter?
>+  prefObserver.addObserver(PREF_XPINSTALL_ENABLED, gExtensionsPrefObserver, false);
>+
Yes, we want a weak reference here for memory mgmt.

>+  // Check the status of XPInstall and enable/disable the update commands
>+  // XXX: is there a cleaner way of doing this?
>+  var cmdUpdate = document.getElementById("cmd_update");
>+  var cmdUpdateAll = document.getElementById("cmd_update_all");
>+  var xpinstallPrefValue = pref.getBoolPref(PREF_XPINSTALL_ENABLED);
>+  if (!xpinstallPrefValue) {
>+    cmdUpdate.setAttribute("disabled", true);
>+    cmdUpdate.setAttribute("update-check", true);
>+    cmdUpdateAll.setAttribute("disabled", true);
>+    cmdUpdateAll.setAttribute("update-check", true);
>+  }
>+
Yes, create a helper function since there are now a couple of places in the
code that set these values. Also, the helper function should use
gExtensionsViewController.updateCommand since the enable case has additional
logic where the command won't be enabled.

> 
> function Shutdown() 
> {
>+  var pref = Components.classes["@mozilla.org/preferences-service;1"]
>+                       .getService(Components.interfaces.nsIPrefBranch2);
>+
>   if (gWindowState != "extensions")
>     gExtensionsView.removeEventListener("select", onThemeSelect, false);
> 
>+  pref.removeObserver(PREF_XPINSTALL_ENABLED, gExtensionsPrefObserver);
>+
nit: it is slightly easier to read if the removeObserver immediately follows
the var pref... there's no reason to separate them.

>+////////////////////////////////////////////////////////////////////////////////
>+// Preference Observer
>+
>+var gExtensionsPrefObserver = {
>+  // aTopic should be an nsISupports instance, aSubject should be "nsPref:changed"
>+  // and aData should be the name of the pref that was changed
>+  observe: function(aTopic, aSubject, aData)
>+  {
>+    // XXX: where can I find nsPref:changed inside nsIPrefBranch2?
>+    if (aSubject != "nsPref:changed") return;
>+    switch (aData) {
>+      case PREF_XPINSTALL_ENABLED : {
>+        var pref = aTopic.QueryInterface(Components.interfaces.nsIPrefBranch);
>+        var cmdUpdate = document.getElementById("cmd_update");
>+        var cmdUpdateAll = document.getElementById("cmd_update_all");
>+
>+        var prefValue = pref.getBoolPref(aData);
>+        if (!prefValue) {
>+          cmdUpdate.setAttribute("disabled", true);
>+          cmdUpdate.setAttribute("update-check", true);
>+          cmdUpdateAll.setAttribute("disabled", true);
>+          cmdUpdateAll.setAttribute("update-check", true);
>+        }
>+        else {
>+          cmdUpdate.removeAttribute("disabled");
>+          cmdUpdate.removeAttribute("update-check");
>+          cmdUpdateAll.removeAttribute("disabled");
>+          cmdUpdateAll.removeAttribute("update-check");
>+        }
>+        break;
>+      }
>+    }
>+    return;
>+  }
>+};

All of this can be greatly simplified by calling a helper function such as
+function toggleUpdateDisallowed(enable)
+{
+  if (enable)
+    gExtensionsView.removeAttribute("update-disallowed");
+  else
+    gExtensionsView.setAttribute("update-disallowed", "true");
+  var command = document.getElementById("cmd_update");
+  gExtensionsViewController.updateCommand(command);
+  command = document.getElementById("cmd_update_all");
+  gExtensionsViewController.updateCommand(command);
+}

then the case statement would only need
+	   var pref = Components.classes["@mozilla.org/preferences-service;1"]
+			       
.getService(Components.interfaces.nsIPrefBranch);
+	   toggleUpdateDisallowed(pref.getBoolPref(PREF_XPINSTALL_ENABLED));

Comment 21

[Robert Strong (they/them - no direct email)]

Forgot to add... also change update-check to something along the lines of
update-disallowed and change the other places in the code that also set the
value to use the helper function.

Comment 22

[Robert Strong (they/them - no direct email)]

One last note - notice the naming in the samnple toggleUpdateDisallowed(enable)
don't really fit well for what it is doing.

Comment 23

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #20)
> (From update of attachment 198076 [details] [diff] [review] [edit])
> This is much closer.
> 
> >+  // Install our preference observer on XPINSTALL_PREF_ENABLED. This fixes
bug 307928.
> >+  // XXX: does the strength of the reference matter?
> >+  prefObserver.addObserver(PREF_XPINSTALL_ENABLED, gExtensionsPrefObserver,
false);
> >+
> Yes, we want a weak reference here for memory mgmt.

Done.

> 
> >+  // Check the status of XPInstall and enable/disable the update commands
> >+  // XXX: is there a cleaner way of doing this?
> >+  var cmdUpdate = document.getElementById("cmd_update");
> >+  var cmdUpdateAll = document.getElementById("cmd_update_all");
> >+  var xpinstallPrefValue = pref.getBoolPref(PREF_XPINSTALL_ENABLED);
> >+  if (!xpinstallPrefValue) {
> >+    cmdUpdate.setAttribute("disabled", true);
> >+    cmdUpdate.setAttribute("update-check", true);
> >+    cmdUpdateAll.setAttribute("disabled", true);
> >+    cmdUpdateAll.setAttribute("update-check", true);
> >+  }
> >+
> Yes, create a helper function since there are now a couple of places in the
> code that set these values. Also, the helper function should use
> gExtensionsViewController.updateCommand since the enable case has additional
> logic where the command won't be enabled.

Ah, that makes sense. The example diff you sent me makes this clear.

Done.

> 
> > 
> > function Shutdown() 
> > {
> >+  var pref = Components.classes["@mozilla.org/preferences-service;1"]
> >+                       .getService(Components.interfaces.nsIPrefBranch2);
> >+
> >   if (gWindowState != "extensions")
> >     gExtensionsView.removeEventListener("select", onThemeSelect, false);
> > 
> >+  pref.removeObserver(PREF_XPINSTALL_ENABLED, gExtensionsPrefObserver);
> >+
> nit: it is slightly easier to read if the removeObserver immediately follows
> the var pref... there's no reason to separate them.

*shrug*

It doesn't really matter to me, that was just where I stuck it.

> 
> >+////////////////////////////////////////////////////////////////////////////////
> >+// Preference Observer
> >+
> >+var gExtensionsPrefObserver = {
> >+  // aTopic should be an nsISupports instance, aSubject should be
"nsPref:changed"
> >+  // and aData should be the name of the pref that was changed
> >+  observe: function(aTopic, aSubject, aData)
> >+  {
> >+    // XXX: where can I find nsPref:changed inside nsIPrefBranch2?
> >+    if (aSubject != "nsPref:changed") return;
> >+    switch (aData) {
> >+      case PREF_XPINSTALL_ENABLED : {
> >+        var pref = aTopic.QueryInterface(Components.interfaces.nsIPrefBranch);
> >+        var cmdUpdate = document.getElementById("cmd_update");
> >+        var cmdUpdateAll = document.getElementById("cmd_update_all");
> >+
> >+        var prefValue = pref.getBoolPref(aData);
> >+        if (!prefValue) {
> >+          cmdUpdate.setAttribute("disabled", true);
> >+          cmdUpdate.setAttribute("update-check", true);
> >+          cmdUpdateAll.setAttribute("disabled", true);
> >+          cmdUpdateAll.setAttribute("update-check", true);
> >+        }
> >+        else {
> >+          cmdUpdate.removeAttribute("disabled");
> >+          cmdUpdate.removeAttribute("update-check");
> >+          cmdUpdateAll.removeAttribute("disabled");
> >+          cmdUpdateAll.removeAttribute("update-check");
> >+        }
> >+        break;
> >+      }
> >+    }
> >+    return;
> >+  }
> >+};
> 
> All of this can be greatly simplified by calling a helper function such as
> +function toggleUpdateDisallowed(enable)
> +{
> +  if (enable)
> +    gExtensionsView.removeAttribute("update-disallowed");
> +  else
> +    gExtensionsView.setAttribute("update-disallowed", "true");
> +  var command = document.getElementById("cmd_update");
> +  gExtensionsViewController.updateCommand(command);
> +  command = document.getElementById("cmd_update_all");
> +  gExtensionsViewController.updateCommand(command);
> +}

OK then, I'll try that.

> 
> then the case statement would only need
> +	   var pref = Components.classes["@mozilla.org/preferences-service;1"]
> +			       
> .getService(Components.interfaces.nsIPrefBranch);
> +	   toggleUpdateDisallowed(pref.getBoolPref(PREF_XPINSTALL_ENABLED));
> 

Right.

Comment 24

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Attachment: 307928-updateUI-v0.3.patch

Picks more of the nits uncovered by Rob.

I would appreciate some feedback about the new XXXs.

Comment 25

[Robert Strong (they/them - no direct email)]

Comment on attachment 198168 [details] [diff] [review]
307928-updateUI-v0.3.patch

This late in the cycle I believe it is better to leave this as is and create a
more complete fix for the trunk.

+  var prefObserver =
pref.QueryInterface(Components.interfaces.nsIPrefBranch2);
+
+  // Install our preference observer on XPINSTALL_PREF_ENABLED. This fixes bug
307928.
+  prefObserver.addObserver(PREF_XPINSTALL_ENABLED, gExtensionsPrefObserver,
true);
lose the comment - the code is self-explanatory.
I was in a rush when I made my earlier comment and misspoke... just pass false
for the holdWeak param.

+  // Check the status of XPInstall and enable/disable the update commands
+  var cmdUpdate = document.getElementById("cmd_update");
+  var cmdUpdateAll = document.getElementById("cmd_update_all");
These aren't used so they should be removed along with the comment


///////////////////////////////////////////////////////////////////////////////
 //
+// Update Command Status Helper
+//
+function toggleUpdateCommands(status)
+{
+  // use 'update-disabled' instead of 'update-disallowed', as the latter will
+  // probably end up being used for something better and/or different
lose the comment

+  if (!status)
+    gExtensionsView.setAttribute("update-disabled");
+  else
+    gExtensionsView.removeAttribute("update-disabled");
+
+  // leverage the updateCommand() function
lose the comment - the code is self-explanatory

+  var command = document.getElementById("cmd_update");
+  gExtensionsViewController.updateCommand(command);
+  command = document.getElementById("cmd_update_all");
+  gExtensionsViewController.updateCommand(command);
+  return;
+}
+
Move all of this to the utility functions section and remove the comment on top

-    command = document.getElementById("cmd_update_all");
-    command.setAttribute("disabled", "true");
+    // turn updates on
lose the comment

-    command = document.getElementById("cmd_update_all");
-    command.removeAttribute("disabled");
+    // turn updates off
same here

+//////////////////////////////////////////////////////////////////////////////
//
+// Preference Observer
+
+var gExtensionsPrefObserver = {
+  // aTopic should be an nsISupports instance, aSubject should be
"nsPref:changed"
+  // and aData should be the name of the pref that was changed
same here

+  observe: function(aTopic, aSubject, aData)
+  {
+    // XXX: where can I find nsPref:changed inside nsIPrefBranch2?
This may be what you are asking
http://lxr.mozilla.org/seamonkey/source/modules/libpref/public/nsIPrefBranch2.i
dl#112
if not, irc is a good place to ask

+    if (aSubject != "nsPref:changed") return;
nit: though the style for this file is 50/50 with placing the return on its own
line I find it easier to read when the retun is on the following line.

+    switch (aData) {
+      case PREF_XPINSTALL_ENABLED : {
+	 var pref = aTopic.QueryInterface(Components.interfaces.nsIPrefBranch);
+	 // use the helper function, as this leverages the logic
+	 // in gExtensionsViewController
This comment isn't necessary.

+	 // XXX: aData or PREF_XPINSTALL_ENABLED?
Either will work since they are equivalent - I prefer aData and though I could
be wrong I believe a lxr search will confirm this.

+	 toggleUpdateCommands(pref.getBoolPref(aData));
+	 break;
+      }
+    }
+    return;
+  },

It would also be simpler to just use
if (aTopic != "nsPref:changed" || aData != PREF_XPINSTALL_ENABLED)
  return;

toggleUpdateCommands(pref.getBoolPref(aData));


btw: if you are unable to find answers to the questions you have irc is a good
place to ask instead of asking in a patch that you are requesting review for.
Also, lxr is a good resource for most of the questions you asked.

Comment 26

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #25)
> (From update of attachment 198168 [details] [diff] [review] [edit])
> This late in the cycle I believe it is better to leave this as is and create a
> more complete fix for the trunk.

Fair enough. Should this bug be downgraded to an 'enhancement' then and
retargeted to trunk?

> 
> +  var prefObserver =
> pref.QueryInterface(Components.interfaces.nsIPrefBranch2);
> +
> +  // Install our preference observer on XPINSTALL_PREF_ENABLED. This fixes bug
> 307928.
> +  prefObserver.addObserver(PREF_XPINSTALL_ENABLED, gExtensionsPrefObserver,
> true);
> lose the comment - the code is self-explanatory.
> I was in a rush when I made my earlier comment and misspoke... just pass false
> for the holdWeak param.

Done.

> 
> +  // Check the status of XPInstall and enable/disable the update commands
> +  var cmdUpdate = document.getElementById("cmd_update");
> +  var cmdUpdateAll = document.getElementById("cmd_update_all");
> These aren't used so they should be removed along with the comment

Whoops. Thanks.

> 
> 
> ///////////////////////////////////////////////////////////////////////////////
>  //
> +// Update Command Status Helper
> +//
> +function toggleUpdateCommands(status)
> +{
> +  // use 'update-disabled' instead of 'update-disallowed', as the latter will
> +  // probably end up being used for something better and/or different
> lose the comment

Done.

> 
> +  if (!status)
> +    gExtensionsView.setAttribute("update-disabled");
> +  else
> +    gExtensionsView.removeAttribute("update-disabled");
> +
> +  // leverage the updateCommand() function
> lose the comment - the code is self-explanatory

Even the comment about the use of the attribute?

> 
> +  var command = document.getElementById("cmd_update");
> +  gExtensionsViewController.updateCommand(command);
> +  command = document.getElementById("cmd_update_all");
> +  gExtensionsViewController.updateCommand(command);
> +  return;
> +}
> +
> Move all of this to the utility functions section and remove the comment on top

OK.

> 
> -    command = document.getElementById("cmd_update_all");
> -    command.setAttribute("disabled", "true");
> +    // turn updates on
> lose the comment
> 
> -    command = document.getElementById("cmd_update_all");
> -    command.removeAttribute("disabled");
> +    // turn updates off
> same here

OK.

> 
> +//////////////////////////////////////////////////////////////////////////////
> //
> +// Preference Observer
> +
> +var gExtensionsPrefObserver = {
> +  // aTopic should be an nsISupports instance, aSubject should be
> "nsPref:changed"
> +  // and aData should be the name of the pref that was changed
> same here

Done.

> 
> +  observe: function(aTopic, aSubject, aData)
> +  {
> +    // XXX: where can I find nsPref:changed inside nsIPrefBranch2?
> This may be what you are asking
> http://lxr.mozilla.org/seamonkey/source/modules/libpref/public/nsIPrefBranch2.i
> dl#112
> if not, irc is a good place to ask

I wish I could use IRC, but thanks anyway.

> 
> +    if (aSubject != "nsPref:changed") return;
> nit: though the style for this file is 50/50 with placing the return on its own
> line I find it easier to read when the retun is on the following line.

OK. I tend to put single statements on the same line because I've seen that done
elsewhere, and it looks better IMHO.

> 
> +    switch (aData) {
> +      case PREF_XPINSTALL_ENABLED : {
> +	 var pref = aTopic.QueryInterface(Components.interfaces.nsIPrefBranch);
> +	 // use the helper function, as this leverages the logic
> +	 // in gExtensionsViewController
> This comment isn't necessary.

OK.

> 
> +	 // XXX: aData or PREF_XPINSTALL_ENABLED?
> Either will work since they are equivalent - I prefer aData and though I could
> be wrong I believe a lxr search will confirm this.

Thanks - this was one of the things I really wanted to know about.

> 
> +	 toggleUpdateCommands(pref.getBoolPref(aData));
> +	 break;
> +      }
> +    }
> +    return;
> +  },
> 
> It would also be simpler to just use
> if (aTopic != "nsPref:changed" || aData != PREF_XPINSTALL_ENABLED)
>   return;
> 
> toggleUpdateCommands(pref.getBoolPref(aData));

OK. I used the switch statement because it would make it easier to add more
prefs to the observer later on, but right now simple is good.

> 
> 
> btw: if you are unable to find answers to the questions you have irc is a good
> place to ask instead of asking in a patch that you are requesting review for.
> Also, lxr is a good resource for most of the questions you asked.
> 

OK. I appreciate the advice and will do so in the future.

Should I submit a new patch with nits picked for the trunk?

Comment 27

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Attachment: 307928-updateUI-v1.0.patch (final)

I decided to post the final, fully-nitpicked version anyway, just so that it
doesn't get lost.

This version should be completely ready for checkin.

NOTE: the comment in gExtensionsPrefObserver about the arguments should be
retained, as the nsIPrefBranch2 idl is completely wrong about the ordering of
the arguments.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #27)
> NOTE: the comment in gExtensionsPrefObserver about the arguments should be
> retained, as the nsIPrefBranch2 idl is completely wrong about the ordering of
> the arguments.

nsIPrefBranch2 has nothing to do with the observer, so I'm not sure what you
mean by this comment. The observer is a nsIObserver, see
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsIObserver.idl#50 .

Comment 29

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #28)
> (In reply to comment #27)
> > NOTE: the comment in gExtensionsPrefObserver about the arguments should be
> > retained, as the nsIPrefBranch2 idl is completely wrong about the ordering of
> > the arguments.
> 
> nsIPrefBranch2 has nothing to do with the observer, so I'm not sure what you
> mean by this comment. The observer is a nsIObserver, see
> http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsIObserver.idl#50 .

The problem is that nsIPrefBranch2 sends the arguments backwards - aTopic is the
'notification specific interface pointer' (aka nsISupports, QIed to
nsIPrefBranch2) and aSubject is the NS_PREF_BRANCH_PREFCHANGE_ID,
"nsPref:changed". According to both IDLs, it ought to be the other way around!

Comment 30

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #29)

> The problem is that nsIPrefBranch2 sends the arguments backwards - aTopic is the
> 'notification specific interface pointer' (aka nsISupports, QIed to
> nsIPrefBranch2) and aSubject is the NS_PREF_BRANCH_PREFCHANGE_ID,
> "nsPref:changed". According to both IDLs, it ought to be the other way around!

http://lxr.mozilla.org/seamonkey/source/modules/libpref/src/nsPrefBranch.cpp#758
looks correct to me.

Per http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsIObserver.idl#50 , it
should be:
observe(aSubject, aTopic, aData)

but you implemented it as:
observe(aTopic, aSubject, aData)

so it's just the names that are mixed up, not the order :).

Comment 31

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #30)
> 
> Per http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsIObserver.idl#50 , it
> should be:
> observe(aSubject, aTopic, aData)
> 
> but you implemented it as:
> observe(aTopic, aSubject, aData)
> 
> so it's just the names that are mixed up, not the order :).

Je suis guepe stupide!

Thanks Gavin.

Comment 32

[Robert Strong (they/them - no direct email)]

Comment on attachment 198563 [details] [diff] [review]
307928-updateUI-v1.0.patch (final)

As discussed over email - a more comprehensive fix (e.g. user notification, new strings, etc.) is preferable on the trunk. It may be simpler to do this after the EM ui has some work done on it as well.

Comment 33

[Robert Strong (they/them - no direct email)]

Fixed by the checkin of bug 329045. The button is still enabled but we show a notification bar with the option to enable xpinstall if the pref is not locked in the same manner as the content window notification. If it is locked the message states this in the same manner as the content window notification.