Firefox still wears the active Persona even it has been removed

VERIFIED FIXED in mozilla1.9.3a5

Status

()

defect
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: whimboo, Assigned: bparr)

Tracking

Trunk
mozilla1.9.3a5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 beta1+)

Details

(Whiteboard: [rewrite], )

Attachments

(2 attachments, 3 obsolete attachments)

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100517 Minefield/3.7a5pre

If you remove an active Persona it will be shown as removed but Firefox still wears it. Shouldn't we switch back to the default theme in such a situation?

Steps:
1. Install the persona given by the above url
2. Open the add-ons manager and select the theme pane
3. Remove the newly installed persona

As you can see after step 3, Firefox still wears the installed persona.
blocking2.0: --- → beta1+
Ben, could you look into making an automated test for this. We'll just test the backend at first, so it's possible you'll find your test passes which would suggest this was a bug in the UI code, but I doubt it.

A few data points to get you started, we can talk about it more when you've had chance to look at it:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_theme.js tests lots of theme and persona related things. You can probably either add to the end of it, or one of the tests in there probably verifies uninstalling a persona works and there is just something we're failing to check.

LightweightThemeManager.currentTheme should tell you what the persona manager thinks the current theme is.

When personas are uninstalled or changed they should be sending a "lightweight-theme-styling-update" notification through the observer service.
Assignee: nobody → bparr
Duplicate of this bug: 567623
Status: NEW → ASSIGNED
Assignee

Comment 3

9 years ago
Posted file XPCShell Test (obsolete) —
Tests that uninstalling a lightweight theme sends a "lightweight-theme-styling-update" notification through the observer service. Currently fails.
Attachment #447586 - Flags: review?(dtownsend)
Assignee

Comment 4

9 years ago
Posted patch XPCShell Test (obsolete) — Splinter Review
Resubmitted as a patch.
Attachment #447586 - Attachment is obsolete: true
Attachment #447587 - Flags: review?(dtownsend)
Attachment #447586 - Flags: review?(dtownsend)
Comment on attachment 447587 [details] [diff] [review]
XPCShell Test

This is great, but I think we could make this test a little more comprehensive. That notification should be sent out whenever the persona changes, currently it is broken for uninstall only but it would be nice to add tests to the other cases to make sure we don't break those in the future. How about making your observer at the top level of the test and verify the flag it sets gets changed whenever a persona is installed, enabled, disabled or uninstalled? This should be in tests 3, 4, 8, 14. You can also check that the notification isn't sent in the other tests.

Can you also check that the theme passed to the notification matches the expected theme? Stash it in a global variable and check its ID is probably the easiest way.
Assignee

Comment 6

9 years ago
Posted patch XPCShell Test v2 (obsolete) — Splinter Review
Made the observer test more comprehensive by including it in each individual test. Expect the notification for tests 3, 4, 8 and 14. Also, expect the notification in test 5 and 6 after the manager restart (i.e. in the check_test_x part). My previously added test 15 turned out to be exactly like test 8, but with a lot more set-up. So I removed test 15. Therefore, test 8 now fails because the notification is not sent when uninstalling a lightweight theme in use.
Attachment #447587 - Attachment is obsolete: true
Attachment #448102 - Flags: review?(dtownsend)
Attachment #447587 - Flags: review?(dtownsend)
Comment on attachment 448102 [details] [diff] [review]
XPCShell Test v2

Looks great to me. Please rename the variable to gLWThemeChanged, we use a g prefix for global variables normally and it could stand to be a little shorter. Rename the observer to LightweightThemeObserver as well. You never call destroy on it that I can see, but that is probably unnecessary so just remove that function.

Next step, fix the code in LightweightThemeManager.jsm so that the test passes!
Attachment #448102 - Flags: review?(dtownsend) → review+
Assignee

Comment 8

9 years ago
Make changes requested by Mossop.
Attachment #448102 - Attachment is obsolete: true
Attachment #448574 - Flags: review?(dtownsend)
Blocks: 461973
No longer blocks: 550048
Attachment #448574 - Flags: review?(dtownsend) → review+
Assignee

Comment 9

9 years ago
Posted patch Simple fixSplinter Review
Attachment #449355 - Flags: review?(dtownsend)
Comment on attachment 449355 [details] [diff] [review]
Simple fix

Looks good to me, we'll get it landed when the tree re-opens.
Attachment #449355 - Flags: review?(dtownsend) → review+
Landed, thanks for the work: http://hg.mozilla.org/mozilla-central/rev/6c3d62593a9e
Flags: in-testsuite+
Flags: in-litmus-
Target Milestone: --- → mozilla1.9.3a5
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Works good so far, except bug 571248 which I have found while verifying this bug.

Marking as verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100610 Minefield/3.7a5pre

I will flag in-litmus? because we will also cover that bug with a general test for Personas installation and removal.
Status: RESOLVED → VERIFIED
Flags: in-litmus- → in-litmus?
Flags: in-litmus? → in-litmus?(vlad.maniac)
Talked with Dave on IRC and we agreed on that no manual test is necessary.
Flags: in-litmus?(vlad.maniac) → in-litmus-
You need to log in before you can comment on or make changes to this bug.