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

VERIFIED FIXED in mozilla2.0b4

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: whimboo, Assigned: mossop)

Tracking

({regression})

Trunk
mozilla2.0b4
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
Assignee: nobody → dtownsend
blocking2.0: ? → betaN+

Comment 1

8 years ago
Bug 135638 is a very old bug (but still open!) about not enabling fresh installed themes.
Blocks: 135638
The EM used to install / select a theme when it was installed and it would be enabled after restart
(Assignee)

Comment 3

8 years ago
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

8 years ago
Created attachment 459699 [details]
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.
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.
(Assignee)

Comment 6

8 years ago
Created attachment 460914 [details] [diff] [review]
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
Attachment #460914 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 7

8 years ago
Created attachment 460955 [details] [diff] [review]
patch rev 2

neglected to remove the permissions entry in the test
Attachment #460914 - Attachment is obsolete: true
Attachment #460955 - Flags: review?(robert.bugzilla)
Attachment #460914 - Flags: review?(robert.bugzilla)
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?
Attachment #460955 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 9

8 years ago
Created attachment 465710 [details] [diff] [review]
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.
Attachment #460955 - Attachment is obsolete: true
Attachment #465710 - Flags: review?(robert.bugzilla)
Attachment #465710 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 10

8 years ago
Created attachment 465735 [details]
hg export

This is a hg export of the patch for landing.
(Assignee)

Updated

8 years ago
Attachment #465735 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 11

8 years ago
This got landed already: http://hg.mozilla.org/mozilla-central/rev/49c7db18003b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
(Reporter)

Comment 12

8 years ago
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100824 Minefield/4.0b5pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.