579779 - Installed new theme doesn't get enabled automatically (no restart prompt)

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100718 Minefield/4.0b2pre

When you install a new theme, it will not be enabled anymore. Instead you have to manually enable it.

Once the theme has been installed, it has to be enabled and the restart notification should appear in the same tab.

Comment 1

[Alfred Kayser]

Bug 135638 is a very old bug (but still open!) about not enabling fresh installed themes.

Comment 2

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

The EM used to install / select a theme when it was installed and it would be enabled after restart

Comment 3

[Dave Townsend [:mossop]]

That is basically an old suite bug and I'm not sure is worth talking about. The problem here is that the new backend never re-implemented bug 408118.

Comment 4

[CatThief]

Attachment: Firefox 4.0b1 Add-ons Manager post theme install

This is what happens after a theme file is dropped into the Add-ons Manager. How is a user supposed to know how to enable it or that a restart is required to complete the process if no button appears for either action?  Simply indicating it is "Ready to install" is not helpful.

Comment 5

[Tony Mechelynck [:tonymec]]

There used to be a line of text in the entry for the newly installed addon: "This add-on will be installed when <application> is restarted." What about re-establishing that? With a little luck, the UI translations will still have the string.

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This enables themes after installation and makes the notification for needing to restart appear. The browser test applies on top of the changes from bug 577048

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 2

neglected to remove the permissions entry in the test

Comment 8

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

Comment on attachment 460955 [details] [diff] [review]
patch rev 2

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -738,17 +738,17 @@ const gXPInstallObserver = {
>       });
>       break;
>     case "addon-install-complete":
>       var notification = PopupNotifications.getNotification(notificationID, browser);
>       if (notification)
>         PopupNotifications.remove(notification);
> 
>       var needsRestart = installInfo.installs.some(function(i) {
>-        return (i.addon.pendingOperations & AddonManager.PENDING_INSTALL) != 0;
>+        return i.addon.pendingOperations != AddonManager.PENDING_NONE;
Why was this only returning true when there was a pending install before?

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -4490,16 +4490,24 @@ AddonInstall.prototype = {
>             }
>             else {
>               XPIProvider.unloadBootstrapScope(self.addon.id);
>             }
>           }
>           AddonManagerPrivate.callAddonListeners("onInstalled",
>                                                  createWrapper(self.addon));
> 
>+          // If installing but not upgrading a theme that is disabled and can
>+          // be enabled then enable it
>+          if (self.addon.type == "theme" && !self.existingAddon &&
>+              self.addon.userDisabled == true && self.addon.appDisabled == false) {
>+            LOG("Enabling new theme");
>+            XPIProvider.updateAddonDisabledState(self.addon, false);
>+          }
This won't enable a theme when installing it if it already exists? If so, that isn't ideal. Perhaps a followup bug?

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 3

(In reply to comment #8)
> Comment on attachment 460955 [details] [diff] [review]
> patch rev 2
> 
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> >--- a/browser/base/content/browser.js
> >+++ b/browser/base/content/browser.js
> >@@ -738,17 +738,17 @@ const gXPInstallObserver = {
> >       });
> >       break;
> >     case "addon-install-complete":
> >       var notification = PopupNotifications.getNotification(notificationID, browser);
> >       if (notification)
> >         PopupNotifications.remove(notification);
> > 
> >       var needsRestart = installInfo.installs.some(function(i) {
> >-        return (i.addon.pendingOperations & AddonManager.PENDING_INSTALL) != 0;
> >+        return i.addon.pendingOperations != AddonManager.PENDING_NONE;
> Why was this only returning true when there was a pending install before?

Previously the only thing a new install would ever be waiting for a restart for was to complete the install. Themes will have completed the install and now will be pending enable. I could just watch for those two cases but it seems more sensible to just show the restart needed stuff when anything is pending a restart.

> >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
> >--- a/toolkit/mozapps/extensions/XPIProvider.jsm
> >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
> >@@ -4490,16 +4490,24 @@ AddonInstall.prototype = {
> >             }
> >             else {
> >               XPIProvider.unloadBootstrapScope(self.addon.id);
> >             }
> >           }
> >           AddonManagerPrivate.callAddonListeners("onInstalled",
> >                                                  createWrapper(self.addon));
> > 
> >+          // If installing but not upgrading a theme that is disabled and can
> >+          // be enabled then enable it
> >+          if (self.addon.type == "theme" && !self.existingAddon &&
> >+              self.addon.userDisabled == true && self.addon.appDisabled == false) {
> >+            LOG("Enabling new theme");
> >+            XPIProvider.updateAddonDisabledState(self.addon, false);
> >+          }
> This won't enable a theme when installing it if it already exists? If so, that
> isn't ideal. Perhaps a followup bug?

Yeah this was trying to avoid the case where a background update installed an updated version of a theme that wasn't in use. We don't want to activate it in that case. I've worked around that though by moving this code to amWebInstallListener.js. I realised that we don't really need to enable themes installed through the raw API, we probably only want to do so for themes installed through webpages and the install dialog. So that's what this does now and will do so regardless of whether there is already a version of the theme installed or not.

Comment 10

[Dave Townsend [:mossop]]

Attachment: hg export

This is a hg export of the patch for landing.

Comment 11

[Dave Townsend [:mossop]]

This got landed already: http://hg.mozilla.org/mozilla-central/rev/49c7db18003b

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100824 Minefield/4.0b5pre