566597 - Firefox still wears the active Persona even it has been removed

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

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.

Comment 1

[Dave Townsend [:mossop]]

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.

Comment 3

[Ben Parr [:bparr]]

Attachment: XPCShell Test

Tests that uninstalling a lightweight theme sends a "lightweight-theme-styling-update" notification through the observer service. Currently fails.

Comment 4

[Ben Parr [:bparr]]

Attachment: XPCShell Test

Resubmitted as a patch.

Comment 5

[Dave Townsend [:mossop]]

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.

Comment 6

[Ben Parr [:bparr]]

Attachment: XPCShell Test v2

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.

Comment 7

[Dave Townsend [:mossop]]

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!

Comment 8

[Ben Parr [:bparr]]

Attachment: XPCShell Test v3

Make changes requested by Mossop.

Comment 9

[Ben Parr [:bparr]]

Attachment: Simple fix

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 449355 [details] [diff] [review]
Simple fix

Looks good to me, we'll get it landed when the tree re-opens.

Comment 11

[Dave Townsend [:mossop]]

Landed, thanks for the work: http://hg.mozilla.org/mozilla-central/rev/6c3d62593a9e

Comment 12

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

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.

Comment 13

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

Talked with Dave on IRC and we agreed on that no manual test is necessary.