Last Comment Bug 690934 - Uninstalling installed language pack reinstalls same language pack on restart
: Uninstalling installed language pack reinstalls same language pack on restart
Status: VERIFIED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 10
Assigned To: Wesley Johnston (:wesj)
:
Mentors:
: 689678 (view as bug list)
Depends on:
Blocks: 689706
  Show dependency treegraph
 
Reported: 2011-09-30 15:02 PDT by Aaron Train [:aaronmt]
Modified: 2012-02-19 07:07 PST (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP Patch (1.50 KB, patch)
2011-09-30 16:51 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Review
WIP Fix again (3.45 KB, patch)
2011-10-03 18:00 PDT, Wesley Johnston (:wesj)
mark.finkle: feedback+
Details | Diff | Review
Nightly (10/04) en-US - installing English (9.73 KB, image/jpeg)
2011-10-04 09:36 PDT, Aaron Train [:aaronmt]
no flags Details
Patch v1 (5.32 KB, patch)
2011-10-04 12:18 PDT, Wesley Johnston (:wesj)
mark.finkle: review+
Details | Diff | Review

Description Aaron Train [:aaronmt] 2011-09-30 15:02:36 PDT
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) 2011-09-30 15:06:24 PDT
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) 2011-09-30 16:51:08 PDT
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] 2011-09-30 17:59:54 PDT
(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 4 Aaron Train [:aaronmt] 2011-10-03 09:36:53 PDT
*** Bug 689678 has been marked as a duplicate of this bug. ***
Comment 5 Wesley Johnston (:wesj) 2011-10-03 18:00:43 PDT
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?) 2011-10-03 20:11:32 PDT
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] 2011-10-04 09:36:10 PDT
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) 2011-10-04 12:18:47 PDT
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 10 Wesley Johnston (:wesj) 2011-10-04 15:51:05 PDT
Backed out because the entire world turned orange https://hg.mozilla.org/integration/mozilla-inbound/rev/00676a1337d0
Comment 11 Wesley Johnston (:wesj) 2011-10-06 09:43:54 PDT
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) 2011-10-06 13:09:24 PDT
pushed again:
http://hg.mozilla.org/integration/mozilla-inbound/rev/57c552ba5654
Comment 13 Ed Morley [:emorley] 2011-10-07 04:12:43 PDT
https://hg.mozilla.org/mozilla-central/rev/57c552ba5654
Comment 14 Aaron Train [:aaronmt] 2011-10-11 07:20:30 PDT
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111011 Firefox/10.0a1 Fennec/10.0a1
Samsung Galaxy SII (Android 2.3.4)

Note You need to log in before you can comment on or make changes to this bug.