690934 - Uninstalling installed language pack reinstalls same language pack on restart

Status: VERIFIED FIXED

Description

[Aaron Train [:aaronmt]]

Using the locale picker, I uninstalled the recently installed locale, restarted and expected to see the fallback to en-US. Instead the same locale reinstalled.

STR:
1. Install en-US Nightly
2. Configure locale URL path *
3. Install any locale and restart
4. Uninstall the locale and restart

ER: Fall back to en-US as a default
AR: Reinstalls the recently uninstalled locale

Is this intended?

--
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20110930 Firefox/10.0a1 Fennec/10.0a1
Galaxy Tab 10.1 (Android 3.1)

* Using http://people.mozilla.org/~wjohnston/nightly_locales.xml
* Using en-US Nightly

Comment 1

[Wesley Johnston (:wesj)]

Doh! Not intentional. I made us smart so we don't do it on disable:

http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/extensions.js#429

but missed the uninstall case.

Comment 2

[Wesley Johnston (:wesj)]

Created attachment 563894 [details] [diff] [review]
WIP Patch

Hmm... I realized this is a bit tougher than I thought. What we really need to do is check if the language being uninstalled is the language you're using. But right now the AddonsManager doesn't give us much information about the locale you're uninstalling, except that it is a locale.

This hackily looks at addon.id and tries to yank out the locale name, which isn't guaranteed to be there. I'm going to ping Mossop and see if he knows a way around this?

Comment 3

[Dave Townsend [:mossop]]

(In reply to Wesley Johnston (:wesj) from comment #2)
> Created attachment 563894 [details] [diff] [review] [diff] [details] [review]
> WIP Patch
> 
> Hmm... I realized this is a bit tougher than I thought. What we really need
> to do is check if the language being uninstalled is the language you're
> using. But right now the AddonsManager doesn't give us much information
> about the locale you're uninstalling, except that it is a locale.
> 
> This hackily looks at addon.id and tries to yank out the locale name, which
> isn't guaranteed to be there. I'm going to ping Mossop and see if he knows a
> way around this?

The AddonsManager doesn't know this, in fact by rights a locale pack could be for any number of locales, all that matters is what is in the chrome.manifest. The only thing I can think of right now is to use the chrome registry to map the url chrome://global/locale/global.dtd to its file uri and see if that is in the add-ons files.

Comment 5

[Wesley Johnston (:wesj)]

Created attachment 564415 [details] [diff] [review]
WIP Fix again

Another WIP, but this time much better. To reiterate what Mossop said, apparently the only way to get this information is to parse the chrome.manifest. There is no JS component sitting around to parse those that I know of. I also asked him today if he would want this code in AddonsManager and he said no.

So this adds a little helper to our extensionsView to do it for us.

Want to cleanup before I send in for review. Need to lazy load NetUtil.jsm and reduce some of the duplicate code in here. Write a little test.

Curious if you have any thoughts mfinkle? I'm only checking for lines like "locale browser en-US", which there can be multiple of in a single addon apparently. Since most of these are single locale packs though, you usually just see a lot of "locale mozapps ar", "locale alerts ar", "locale places ar", etc.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 564415 [details] [diff] [review]
WIP Fix again

Looks pretty good. You could just move the Cu.import into _getLocalesInAddon, since it's the only caller of NetUtil.

Comment 7

[Aaron Train [:aaronmt]]

Created attachment 564586 [details]
Nightly (10/04) en-US - installing English

Side effect from this on 10/04 -- I get a dialog that Fennec is "Installing English"! (See screenshot). I didn't see this before. Did something related land?

STR: Install romania, restart, uninstall Romania.

See screenshot.

Comment 8

[Wesley Johnston (:wesj)]

Created attachment 564634 [details] [diff] [review]
Patch v1

Ready for review. This is pretty much the same with a little cleanup.

Also added a test on the _getLocalesInAddon function. Ideally we could actually install the addon, change languages, uninstall the addon, and see what happened. Unfortunately bootstrapped locales are not yet supported, no matter what you put in your install.rdf file.

I have that test written, and could mark as todo is if that is better, but this just tests the general function I wrote vs. a generic chrome.manifest with some valid and invalid entries.

Comment 9

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0a11abbdc4d9

Comment 10

[Wesley Johnston (:wesj)]

Backed out because the entire world turned orange https://hg.mozilla.org/integration/mozilla-inbound/rev/00676a1337d0

Comment 11

[Wesley Johnston (:wesj)]

I pushed both patches separately to try yesterday and I'm 99% sure the other guy caused all the orange, but there seemed to be a lot of failures with this as well... I pushed it to try one more time to see if they were just randoms or not.

Comment 12

[Wesley Johnston (:wesj)]

pushed again:
http://hg.mozilla.org/integration/mozilla-inbound/rev/57c552ba5654

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/57c552ba5654

Comment 14

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111011 Firefox/10.0a1 Fennec/10.0a1
Samsung Galaxy SII (Android 2.3.4)