Closed Bug 562919 Opened 14 years ago Closed 14 years ago

When migrating with multiple themes all will be marked as enabled in the database

Categories

(Toolkit :: Add-ons Manager, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a5

People

(Reporter: alfredkayser, Assigned: mossop)

References

Details

(Keywords: testcase, Whiteboard: [AddonsRewriteTestday][rewrite])

Attachments

(5 files)

In the new addons UI introduced by 554007, the 'Use Theme' button is missing.
Only 'Personas' can be enabled, but then they cannot be disabled.
Blocks: 554007
Enable on the default theme (I only have one so just using that as an example) will unload the persona.
There is no 'enable' button on other full themes. That is the real issue here.
Blocks: 550048
Whiteboard: [AddonsRewriteTestday]
I see an enable button. Can you please give exact steps and tell us your complete build id? Thanks.
No longer blocks: 554007
Attached image Enable Themes button
I also see the enable button on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100430 Minefield/3.7a5pre.  It's right there for your walnut theme, yay!

Perhaps this is OS dependent issue?
This is with build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100430 Minefield/3.7a5pre GTB5

This is in Windows XP, classic theme.
To be more precise,
the .addon-control.enable button has: hidden="true".
So, somehow the logic of the addon manager makes it hidden.
I have seen it too a couple of minutes ago. I have no steps yet but will keep an eye on it.
I have still no idea why that happens but I have a simplified profile ready which I will attach to this bug now.
Summary: With the new addons UI, there is no way to use a theme. → Under some conditions it's not possible to enable other themes (personas)
Version: unspecified → Trunk
Keywords: testcase
Whiteboard: [AddonsRewriteTestday] → [AddonsRewriteTestday][rewrite]
Attached file test profile
Just create a fresh profile and replace its content. Then open the Addons Manager and select the Themes pane. As you can see there is no Enable button for the Default theme.
This sounds like a manifestation of my [suite] issue.

Where if there is more than one "app-installed" theme, there is no enable button on the other.

I suspect the manager is making some assumption about "App Theme is Enabled" and does not provide buttons for the _other_ app themes. If this is a separate issue I can split it out if need be
Blocks: SMAddonMgr
This extensions.sqlite was created by running an about:addons build once on an existing profile.
Priority: -- → P1
Blocks: 461973
No longer blocks: 550048
(In reply to comment #9)
> Created an attachment (id=442850) [details]
> test profile
> 
> Just create a fresh profile and replace its content. Then open the Addons
> Manager and select the Themes pane. As you can see there is no Enable button
> for the Default theme.

I think this profile is causing a different bug which I've filed as bug 563971. The others here seem to be seeing something different.
(In reply to comment #5)
> Created an attachment (id=442793) [details]
> Screenshot showing the missing 'enable' button on themes
> 
> This is with build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
> rv:1.9.3a5pre) Gecko/20100430 Minefield/3.7a5pre GTB5
> 
> This is in Windows XP, classic theme.

For some reason here the manager thinks that Walnut is enabled, but I presume the default theme is the one that is showing? The other themes there don't have enable buttons because they are incompatible with the app I think.
Figured this out. This is a problem when migrating multiple themes from the old extensions.rdf. We never used to make a userDisabled property in the rdf for themes, the preference was all that was used. The current code assumes a lack of userDisabled property means the add-on was enabled which is wrong for themes.
Summary: Under some conditions it's not possible to enable other themes (personas) → When migrating with multiple themes all will be marked as enabled in the database
Attached patch patch rev 1Splinter Review
This solves the problem at migration time and adds an automated test for it. It doesn't currently fix the problem for existing users that have hit it, need to consider the cost of that vs. just asking them to delete the extensions.sqlite from their profile.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #444177 - Flags: review?(robert.bugzilla)
http://hg.mozilla.org/projects/addonsmgr/rev/6fbb50534c71
Flags: in-testsuite+
Whiteboard: [AddonsRewriteTestday][rewrite] → [AddonsRewriteTestday][rewrite][fixed-in-addonsmgr][needs-review]
Comment on attachment 444177 [details] [diff] [review]
patch rev 1

>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
>@@ -1383,17 +1383,20 @@ var XPIProvider = {
>       // Update the AddonInternal properties.
>       newAddon._installLocation = aInstallLocation;
>       newAddon.visible = !(newAddon.id in visibleAddons);
>       newAddon.installDate = aAddonState.mtime;
>       newAddon.updateDate = aAddonState.mtime;
> 
>       // If there is migration data then apply it.
>       if (aMigrateData) {
>-        newAddon.userDisabled = aMigrateData.userDisabled;
>+        // Theme disabled state is determined by the selected theme preference
>+        // and will have been set in loadManifestFromRDF
Perhaps something along these lines?
// A theme's disabled state is determined by the selected theme preference
// which is read in loadManifestFromRDF

>+        if (newAddon.type != "theme")
>+          newAddon.userDisabled = aMigrateData.userDisabled;
>         if ("installDate" in aMigrateData)
>           newAddon.installDate = aMigrateData.installDate;
>         if ("targetApplications" in aMigrateData)
>           newAddon.applyCompatibilityUpdate(aMigrateData, true);
>       }
> 
>       // Update the database.
>       XPIDatabase.addAddonMetadata(newAddon, aAddonState.descriptor);

r=me with the wording fixed up
Attachment #444177 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [AddonsRewriteTestday][rewrite][fixed-in-addonsmgr][needs-review] → [AddonsRewriteTestday][rewrite][fixed-in-addonsmgr][needs-landing]
http://hg.mozilla.org/mozilla-central/rev/2a012e312acc
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [AddonsRewriteTestday][rewrite][fixed-in-addonsmgr][needs-landing] → [AddonsRewriteTestday][rewrite]
Target Milestone: --- → mozilla1.9.3a5
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100517 Minefield/3.7a5pre

Dave, do the automated test cover all of the possible cases or do we also need manual tests?
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Should be fine here.
Flags: in-litmus? → in-litmus-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: