385787 - Gray-out restart firefox button on installing theme/extension updates window

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Tony Chung [:tchung]]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a6pre) Gecko/20070625 Minefield/3.0a6pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a6pre) Gecko/20070625 Minefield/3.0a6pre

I think we should gray out the "restart firefox" button when the user is trying to install a update to themes and extensions.   Call the attention to the "Install updates" button, or you will confuse the user to restart firefox instead, thinking it will install the updates by simply restarting.

See attachment.

Reproducible: Always

Steps to Reproduce:
1. add a extension or theme to a prior version of FF, that supports the latest alpha (eg. myFirefox theme)
2. update to the latest nightly build
3. open up addon manager, and do a "Find updates" on the theme or extension
4. When update is available for the theme / extension, go to Updates dialog
5. Verify the "Restart Firefox" button is active, but we should call the user to attention on the "Install updates" first.

See attachment.
Actual Results:  
Restart Firefox button is active on the install updates window

Expected Results:  
Should gray out the Restart Firefox button until the Update has been installed.

Comment 1

[Tony Chung [:tchung]]

Attachment: restart button not grayed out

Comment 2

[Simon Bünzli]

(In reply to comment #0)
> Should gray out the Restart Firefox button until the Update has been installed.

... or another extension was enabled/disabled/uninstalled or a new theme was selected/uninstalled. Even with updates pending, you might want to do other extension related stuff first.

BTW: Extensions should be able to easily enable that button from their Options dialog as well when they need it - e.g. through

window.opener.getElementById("restartAppButton").disabled = false;

Comment 3

[Dão Gottwald [:dao]]

I can't open attachment 269705 [details].

(In reply to comment #2)
> BTW: Extensions should be able to easily enable that button from their Options
> dialog as well when they need it - e.g. through
> 
> window.opener.getElementById("restartAppButton").disabled = false;

Well, that's a hack. The state would get lost when doing other stuff in the Add-ons manager.

Comment 4

[Dave Townsend [:mossop]]

I think if add-ons need a restart for some of their options then they should handle it themselves, it's easy enough. This button should be for restarting due to changes in the add-ons window, that is add-on installs/uninstalls/enables/disables or theme switches.

Comment 5

[Tony Chung [:tchung]]

dao-- sorry, i accidently checked the private button on the attachment.  it's viewable now.

Comment 6

[Dão Gottwald [:dao]]

Attachment: gray out restart button

Comment 7

[Dão Gottwald [:dao]]

Attachment: gray out restart button

Comment 8

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

Since we display the Restart button all the time I think we should move the Install Updates button to the left. This would make it consistent with the location of the Find Updates button and just look better overall IMO. Dão, could you add that to this patch?

Comment 9

[Dão Gottwald [:dao]]

Attachment: gray out restart button, move install button to the left

Comment 10

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

Comment on attachment 270347 [details] [diff] [review]
gray out restart button, move install button to the left

I did a quick test of this patch and it appears that if you switch to any other view while an extension is being updated the restart button is enabled. r-'ing based on that.

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> (From update of attachment 270347 [details] [diff] [review])
> I did a quick test of this patch and it appears that if you switch to any other
> view while an extension is being updated the restart button is enabled.

Isn't that what gExtensionsView.hasAttribute("update-operation") is there for? I don't understand how the patch could make a difference for that matter.

Comment 12

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

The logic used for enabling / disabling the restart button changed with the landing of bug 369075.

Comment 13

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

To be more specific, we didn't display a restart button in the other views and now we do. Now the code for enabling / disabling it will need to be updated accordingly.

Comment 14

[Dão Gottwald [:dao]]

Attachment: gray out restart button, move install button to the left v2

Comment 15

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

Comment on attachment 271807 [details] [diff] [review]
gray out restart button, move install button to the left v2

I'm going to only review the portions of this patch that are actually applicable to fix this bug. The majority if not all of the remaining changes can be fixed as those parts of the file are patched.

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 271807 [details] [diff] [review]
gray out restart button, move install button to the left v2

Rob asked me to take a look over this. Generally looks good to me however I came across one issue:

>+var gInstalling       = 0;

This assumes that there are no install operations ongoing when the add-ons window is opened. In the event that there are the restart buttons will be incorrectly enabled. You need to initialise this to an appropriate value in Startup.

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #16)
> This assumes that there are no install operations ongoing when the add-ons
> window is opened. In the event that there are the restart buttons will be
> incorrectly enabled. You need to initialise this to an appropriate value in
> Startup.

I don't see a direct way to get the number of installing extensions or an easy way to check all the extensions' states. So I'm not sure if I'll be able to address that. If it can be left to a new bug, that would be my preferred way to proceed. It's only remotely related to the original bug anyway, which was about the "installing updates" view specifically.

Comment 18

[Dave Townsend [:mossop]]

This is really just the same issue as that from comment 10 in different circumstances so I think we want to take care of this here. True there is no direct method to just get a count but it is not difficult to work out. Just check all the add-ons for a state property. If they have one and it isn't success or failure then count it as an install.

updateOptionalViews already iterates all the add-ons and is called from Startup so I guess you could potentially use that to initialise the count, it shouldn't matter that it gets recalculated occasionally while the window is open.

Comment 19

[Dão Gottwald [:dao]]

(In reply to comment #18)
> This is really just the same issue as that from comment 10 in different
> circumstances so I think we want to take care of this here.

Indeed, your review- wasn't less justified than the one before that. But I think comment 10 already goes beyond what this bug was about.

> updateOptionalViews already iterates all the add-ons and is called from Startup
> so I guess you could potentially use that to initialise the count, it shouldn't
> matter that it gets recalculated occasionally while the window is open.

Ah, I must have missed that function. I'll take a look at it.

Comment 20

[Dão Gottwald [:dao]]

Attachment: patch v3

Disclaimer: While the previous patch was tested, this one isn't. I'm also not familiar with RDF.

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 273764 [details] [diff] [review]
patch v3

This is pretty much there I think, one issue though, since updateOptionalViews can be called while the window is open you need to set gInstalling to 0 at the start of it to avoid doubling up the count.

A couple of other nits:

>+var gInstalling       = 0;

Sorry, should have thought of this before, maybe gInstallCount is more descriptive?

>-  var elements = ctr.GetElements();
>+  var elements = ctr.GetElements(),
>+      showLocales = false,
>+      showPlugins = false,
>+      showUpdates = false,
>+      showInstalls = false;

Looks like you have tabs or something and please declare with var to avoid strict warnings.

>+    if (state) {
>+      state = state.QueryInterface(Components.interfaces.nsIRDFLiteral);

Better I think is just:
if (state && state instanceof Components.interfaces.nsIRDFLiteral)

>+      if (state.Value && state.Value != "success" && state.Value != "failure")

Shouldn't be any need for the first check of state.Value.

With those changes this will be fine by me, please request the final review from rob since he has to have the final say.

Comment 22

[Dão Gottwald [:dao]]

(In reply to comment #21)
> >-  var elements = ctr.GetElements();
> >+  var elements = ctr.GetElements(),
> >+      showLocales = false,
> >+      showPlugins = false,
> >+      showUpdates = false,
> >+      showInstalls = false;
> 
> Looks like you have tabs or something and please declare with var to avoid
> strict warnings.

It's all the same statement (-> commas at the end of the lines).

Comment 23

[Dave Townsend [:mossop]]

Oops sorry, missed that. My personal preference would be to change it to multiple statements (for the very reason that I think it's less clear), but I guess I'm happy either way.

Comment 24

[Dão Gottwald [:dao]]

Attachment: patch with Dave's comments addressed

Comment 25

[Dão Gottwald [:dao]]

Attachment: patch with Dave's comments addressed

bogus whitespace removed

Comment 26

[Dão Gottwald [:dao]]

Attachment: patch v4

because of the code removal in updateGlobalCommands(), the loop can be optimized

Comment 27

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

Comment on attachment 273779 [details] [diff] [review]
patch v4

>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v
>retrieving revision 1.128
>diff -u -8 -p -r1.128 extensions.js
>--- toolkit/mozapps/extensions/content/extensions.js	19 Jul 2007 18:14:05 -0000	1.128
>+++ toolkit/mozapps/extensions/content/extensions.js	25 Jul 2007 13:23:16 -0000
>...
>@@ -1371,74 +1379,85 @@ function canWriteToLocation(element)
>...
>+    var stateArc = rdfs.GetResource(PREFIX_NS_EM + "state");
>+    var state = ds.GetTarget(e, stateArc, true);
>+    if (state && state instanceof Components.interfaces.nsIRDFLiteral &&
>+        state.Value != "success" && state.Value != "failure")
>+      gInstallCount++;
>+
>     if (!showUpdates) {
>       var updateURLArc = rdfs.GetResource(PREFIX_NS_EM + "availableUpdateURL");
>       var updateURL = ds.GetTarget(e, updateURLArc, true);
>       if (updateURL) {
>         var updateableArc = rdfs.GetResource(PREFIX_NS_EM + "updateable");
>         var updateable = ds.GetTarget(e, updateableArc, true);
>         updateable = updateable.QueryInterface(Components.interfaces.nsIRDFLiteral);
>         if (updateable.Value == "true")
>-          var showUpdates = true;
>+          showUpdates = true;
>       }
>     }
> 
>     if (showInstalls)
>       continue;
> 
>     var stateArc = rdfs.GetResource(PREFIX_NS_EM + "state");
>     var state = ds.GetTarget(e, stateArc, true);
>     if (state)
>-      var showInstalls = true;
>+      showInstalls = true;
This should be rewritten into the previous state check to remove the optimization to not check state since we alsways check state for gInstallCount. Something like:

>+    var stateArc = rdfs.GetResource(PREFIX_NS_EM + "state");
>+    var state = ds.GetTarget(e, stateArc, true);
>+    if (state) {
>+      showInstalls = true;
>+      if (state instanceof Components.interfaces.nsIRDFLiteral &&
>+          state.Value != "success" && state.Value != "failure")
>+        gInstallCount++;
>+    }
>+
>     if (!showUpdates) {
>       var updateURLArc = rdfs.GetResource(PREFIX_NS_EM + "availableUpdateURL");
>       var updateURL = ds.GetTarget(e, updateURLArc, true);
>       if (updateURL) {
>         var updateableArc = rdfs.GetResource(PREFIX_NS_EM + "updateable");
>         var updateable = ds.GetTarget(e, updateableArc, true);
>         updateable = updateable.QueryInterface(Components.interfaces.nsIRDFLiteral);
>         if (updateable.Value == "true")
>-          var showUpdates = true;
>+          showUpdates = true;
>       }
>     }
> 
>-    if (showInstalls)
>-      continue;
>-
>-    var stateArc = rdfs.GetResource(PREFIX_NS_EM + "state");
>-    var state = ds.GetTarget(e, stateArc, true);
>-    if (state)
>-      var showInstalls = true;

Almost there... the next one should do it

btw: when you have lots of whitespace changes and I'm the reviewer I'd appreciate a diff -w

Comment 28

[Dão Gottwald [:dao]]

Attachment: patch v5

Comment 29

[Dão Gottwald [:dao]]

Attachment: patch v5, diff -w

Comment 30

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

Comment on attachment 275698 [details] [diff] [review]
patch v5

Thanks

Comment 31

[Dão Gottwald [:dao]]

finally :)

Comment 32

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

Checked in to trunk

Checking in mozilla/toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.130; previous revision: 1.129
done
Checking in mozilla/toolkit/mozapps/extensions/content/extensions.xul;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xul,v  <--  extensions.xul
new revision: 1.59; previous revision: 1.58
done

Comment 33

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007092705 Minefield/3.0a9pre.