259203 - uninstalling current non-default theme breaks browser

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Pierre Chanial]

Steps to reproduce:

- install a theme
- restart
- uninstall the current theme
- restart

Browser is completely horked, there's no way to select any theme, the extension
manager is broken. There is no easy way to get around it, except by deleting the
Extension.rdf file and "lose" the extensions.

Uninstalling the current theme should select the default one.

pch

Comment 1

[Asa Dotzler [:asa]]

I can't reproduce this on windows. It won't let me uninstall the current theme.
It seems like it does, but on restart, that selected (supposedly uninstalled)
theme is still my theme.  tested this morning's bits on windows2K

Comment 2

[Asa Dotzler [:asa]]

marcia and I could reliably reproduce bug 258892. Is this related?

Comment 3

[Pierre Chanial]

I retested on today branch build and got the same problems.
That's the same steps as in bug 258892. Whereas the windows bug is inocuous,
linux breaks badly on this one.
I attached a screenshot in bug 258892 because I was about to dupe it, but the
behaviours are so different on the two platforms that I am not sure it is a good
idea to do it.

Comment 4

[Benjamin Smedberg]

Do you have chrome JS errors logged, and does anything show up in the JS console?

Comment 5

[Benjamin Smedberg]

OK, this can be fixed in chromereg, "UninstallProvider" needs a little bit more
love. This could also use some EM love, and I might be able to fix that also.

Comment 6

[Benjamin Smedberg]

Attachment: Part 1 - chromereg goodness

Instead of hardcoding a resource urn:mozilla:locale:en-US we need to loop
through the locales from urn:mozilla:locale:root and find a matching locale.
This patch also falls back to matching languages which don't have matching
country codes, for example you can set "en-GB" as your preferred locale, and
still select the "en-US" locale for extensions that only ship it.

Comment 7

[Benjamin Smedberg]

Comment on attachment 159994 [details] [diff] [review]
Part 1 - chromereg goodness

darin, this is mostly string-fu, so you get to do the honors again

Comment 8

[Darin Fisher]

Comment on attachment 159994 [details] [diff] [review]
Part 1 - chromereg goodness

>Index: nsChromeRegistry.cpp

>+    // "a" is short
>+    if (as == ae) {
>+      if (*++bs == '-')
>+        return PR_TRUE;
>+
>+      return PR_FALSE;
>+    }

nit: you could compress this code down to just this:

      if (as == ae)
	return *++bs == '-';


> nsresult
> nsChromeRegistry::FindSubProvider(const nsACString& aPackage,
>                                   const nsACString& aProvider,
>                                   nsCOMPtr<nsIRDFResource> &aSelectedProvider)
> {
>   nsresult rv;
> 
>   PRBool isLocale = aProvider.Equals(NS_LITERAL_CSTRING("locale"));

nit: use EqualsLiteral?  (w/ code as is for the branches)


>+    if (name.Equals(selectedProvider))
>+      thisQuality = PRECISE;
>+    else if (isLocale && LanguagesMatch(name, selectedProvider))
>+        thisQuality = LANGUAGE;

nit: indentation inconsistency


>+    if (thisQuality <= quality) continue;
>+
>+    rv = TrySubProvider(aPackage, kid, aSelectedProvider);
>+    if (NS_FAILED(rv)) continue;

might be nice to put the |continue| statements on a separate
line to faciliate setting breakpoints in a debugger :-/


>+  // we found the locale, cache it in memory

locale _or skin_ right?

Comment 9

[Benjamin Smedberg]

Attachment: Patch actually checked in on trunk

This is the patch actually checked in on the trunk. I'm going to leave this bug
open for "part 2", the extension manager UI changes.

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 160057 [details] [diff] [review]
Patch actually checked in on trunk

a=asa for aviary checkin.

Comment 11

[timeless]

so if you have two languages which are not remotely compatible but share a
common prefix, you'll use the one that doesn't interest the user if the one the
user wants is unavailable?

Comment 12

[Benjamin Smedberg]

Part 1 checked in on branch. Still leaving this open for UI changes in the theme
manager.

Comment 13

[Benjamin Smedberg]

ok, I took a quick look through the theme manager, and I don't feel comfortable
catching all the permutations of uninstall. Reassigning to ben for the part 2
fix (disabling the uninstall button for selected themes).

Comment 14

[Benjamin Smedberg]

Ben fixed the FE half of this bug in bug 258892