677092 - Make language packs restartless by default

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Axel Hecht [:Pike]]

Now that we can register chrome with restartless add-ons, we should make langpacks restartless by default.

Probably much easier to do once bug 591780 is done, thus marking a dependency.

Comment 1

[Tony Mechelynck [:tonymec]]

Changing UI locale on the fly? That would be a nice touch. :-)

Comment 2

[Dave Townsend [:mossop]]

(In reply to Tony Mechelynck [:tonymec] from comment #1)
> Changing UI locale on the fly? That would be a nice touch. :-)

That should be considered a separate bug (which would at least depend on bug 377881)

Comment 3

[Benjamin Smedberg]

Separate, but already possible: so once you have restartless langpacks, locale switcher should be able to use them!

Comment 4

[Tony Mechelynck [:tonymec]]

On SeaMonkey (which I use), ui-locale switching is governed not only by the -ui-locale command-line switch, but also (alternatively) by Edit → Preferences → Appearance → User Interface Language. Once langpacks are restartless, this dropdown ought to be able to switch them on the fly.

Comment 5

[(no longer active)]

Bug 591780 is about spellcheck dictionaries, not langpacks.  Sorry!

Comment 6

[Axel Hecht [:Pike]]

I filed this bug to be dependent on bug 591780 because that provides the code path in the addons manager in which langpacks would just need to pile on. At least that's how I read the addons manager patch at the time.

Comment 7

[(no longer active)]

I haven't looked at the add-on manager parts of that patch correctly, but I won't fight over a dependency!  I definitely hope that the code added there can be used here as well.  :-)

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

hmm, does it mean that we can retranslate XUL on fly?

Comment 9

[Axel Hecht [:Pike]]

Nope, this is only from when on in the install process you can select the new language, not switching it on or what happens when you do.

Comment 10

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120320 Firefox/14.0a1 SeaMonkey/2.11a1 ID:20120320003047

I notice that when updating langpacks on trunk, since yesterday or maybe the day before the add-on manager no longer says "will be updated/installed at next restart" but rather installs them immediately. I don't know what fixed this bug though, so I'm setting it VERIFIED WORKSFORME for the time being. Feel free to set it FIXED instead if you can supply the fixing changeset or bug ID.

Comment 11

[Axel Hecht [:Pike]]

Reopening. I just tried to install a language pack on nightly, 
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:14.0) Gecko/20120320 Firefox/14.0a1
from ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n/mac/xpi/ and it asks for restart to install.

Comment 12

[Tony Mechelynck [:tonymec]]

(In reply to Axel Hecht [:Pike] from comment #11)
> Reopening. I just tried to install a language pack on nightly, 
> Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:14.0) Gecko/20120320
> Firefox/14.0a1
> from
> ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-
> l10n/mac/xpi/ and it asks for restart to install.

Ah. Well those I update from my HD after download from ftp://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/latest-comm-central-trunk-l10n/*/xpi (where * is one of win32, mac, linux-i686) it doesn't. I wonder why the difference. (Download via command-line ftp then install from HD allows me to install them wholesale, all 21 [for SeaMonkey] in one step.)

Comment 14

[Paul [pwd]]

I recently (this week had to reinstalled my en-GB dictionary and was asked to restart which prompted me to file bug 759088. This definitely isn't fixed.

Comment 15

[Axel Hecht [:Pike]]

Paul, dictionaries and language packs are different things. Please keep those discussions separate. Dictionaries were implemented in bug 591780. If that's not working for you, that's the bug to comment on.

Comment 16

[Paul [pwd]]

Apologies.

Comment 17

[Axel Hecht [:Pike]]

Not really depending on dictionary code, but it helped.

I've got a local patch in the works, so taking.

Comment 18

[Axel Hecht [:Pike]]

Attachment: make langpacks restartless

Interstingly, this is all it takes to make langpacks behave like I'd love them to. They install restartless, the uninstall fine, and they kick off changes if you reinstall an updated version of a language pack.

The patch doesn't have a single test, mossop, can you help with that?

Comment 19

[Axel Hecht [:Pike]]

... also, I didn't need a boostrap.js, but I'm not sure if the warning for the various methods failing is appreciated, or if we should silence those with a no-op bootstrap.js

Comment 20

[Dave Townsend [:mossop]]

(In reply to Axel Hecht [:Pike] from comment #18)
> Created attachment 651494 [details] [diff] [review]
> make langpacks restartless
> 
> Interstingly, this is all it takes to make langpacks behave like I'd love
> them to. They install restartless, the uninstall fine, and they kick off
> changes if you reinstall an updated version of a language pack.

Can you describe how this feature is expected to work? If I was a user installing a language pack, and it completed without restart I'd expect the UI to switch to the new landing immediately, but unless I'm missing some magic I don't think that'll happen with this patch will it?

Comment 21

[Axel Hecht [:Pike]]

Locale-switching is out of context of this bug, but if you do switch locale manually via about:config or another add-on, just opening a new window is good enough to pick up new locales, or changes to existing ones.

I'm staying away from putting locale switching behaviour into langpacks itself as you can end up installing multiple languages, and, AFAICT, the matchOS linux distros use our langpack logic to install global bundles, too.

That said, can the add-on manager detect that a user action installs one and only one langpack manually? I'd like to restrict it to manually, as I'm hacking on this in the context of many-locales builds for mobile, where we want to extract the toolkit strings and install them dynamically in the background if needed via langpacks. I'd rather not end up in a state there where gecko and native UI end up with incompatible settings with native assuming matchOS and gecko setting locale explicitly.

Comment 22

[Dave Townsend [:mossop]]

(In reply to Axel Hecht [:Pike] from comment #21)
> Locale-switching is out of context of this bug, but if you do switch locale
> manually via about:config or another add-on, just opening a new window is
> good enough to pick up new locales, or changes to existing ones.

Ah right, I see.

> That said, can the add-on manager detect that a user action installs one and
> only one langpack manually? I'd like to restrict it to manually, as I'm
> hacking on this in the context of many-locales builds for mobile, where we
> want to extract the toolkit strings and install them dynamically in the
> background if needed via langpacks. I'd rather not end up in a state there
> where gecko and native UI end up with incompatible settings with native
> assuming matchOS and gecko setting locale explicitly.

Manually basically means at runtime? I think it's possible to detect that.

Comment 23

[Dave Townsend [:mossop]]

(In reply to Axel Hecht [:Pike] from comment #19)
> ... also, I didn't need a boostrap.js, but I'm not sure if the warning for
> the various methods failing is appreciated, or if we should silence those
> with a no-op bootstrap.js

Yeah we should probably silence those. If you just return early from callBootstrapMethod if aType == "locale" it should do it I think. http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#3672

Comment 24

[Dave Townsend [:mossop]]

Comment on attachment 651494 [details] [diff] [review]
make langpacks restartless

Looks good with the previous comment. Blair should do a final review here and probably has more time to help you out with tests than I do right now, though test_dictionary.js ought to be a good place to start (you can throw away all the dictionary bits of that of course).

Comment 25

[Axel Hecht [:Pike]]

Attachment: make langpacks restartless, no bootstrap.js. and no tests

Hi Blair, this patch does the right thing, I think.

It went through try (with a bastard commit message) on https://tbpl.mozilla.org/?tree=Try&rev=7b614bd3b6c4.

I'd appreciate if you could help out on the testing side.

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 656392 [details] [diff] [review]
make langpacks restartless, no bootstrap.js. and no tests

Review of attachment 656392 [details] [diff] [review]:
-----------------------------------------------------------------

Seems ok, given the odd requirements for langpacks. So, for testing...

You want a locale addon added to /toolkit/mozapps/extensions/test/addons/ - any folder in there with an install.rdf gets packaged as an XPI in the build process. Can base it on this:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/addons/test_install2_2/

Note that the test_locale addon there isn't actually a locale addon. And the test_locale.js test isn't about testing locale addons.

And add an xpcshell test in /toolkit/mozapps/extensions/test/xpcshell/ - modeled roughly on http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_dictionary.js
There's a bunch of that test that isn't needed for this, you really only need:

run_test_1
check_test_1
run_test_2
run_test_4
run_test_7
check_test_7

Everything else is tested well enough elsewhere. And of course remove any dictionary specific stuff, and anything to do with the http.js server.
I don't think we have a way to test that locale entries in chrome.manifest are correctly registered.

Comment 27

[Axel Hecht [:Pike]]

Attachment: tests for the patch

This is how far I got with tests, I'd need further help on two items:

The crash reporter stuff doesn't seem to mention langpacks, is that intentional? I left those commented out for now.

The do_check_true(true) are the original dictionary tests that I kept for reference.

My main problem right now is the negative assertion, aka, to assert that the langpack isn't active.

The chrome registry throws on getSelectedLocale, which mostly works allright. But I see a failure about the addons manager not being initialized, if I just comment out the chrome reg try{} block, the test passes. Here's the output:

### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /var/folders/w7/6425lz1168gg9h8nh7jgc5vw0000gn/T/firefox/xpcshellprofile/runxpcshelltests_leaks.log

TEST-INFO | (xpcshell/head.js) | test 1 pending

TEST-INFO | (xpcshell/head.js) | test 2 pending
*** LOG addons.manager: Application has been upgraded
*** LOG addons.xpi: startup
*** LOG addons.xpi: checkForChanges
*** LOG addons.xpi: No changes found

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 902] 0 == 0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 903] onNewInstall == onNewInstall

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [ensure_test_completed : 1015] 0 == 0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 41] [object Object] != null

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 42] locale == locale

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 43] 1.0 == 1.0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 44] Language Pack x-testing == Language Pack x-testing

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 45] 3 == 3

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 46] true == true

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 47] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 49] 0 == 0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 935] 5 == 5

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 936] 0 == 0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 937] onInstallStarted == onInstallStarted
*** LOG addons.xpi: Starting install of file:///Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/addons/test_langpack.xpi

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 862] onInstalling == onInstalling

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 863] false == false
*** LOG addons.xpi: Addon [e-mail] will be installed as an unpacked directory
*** LOG addons.xpi-utils: Opening database
*** LOG addons.xpi-utils: Creating database schema

TEST-INFO | (xpcshell/head.js) | test 2 finished

TEST-INFO | (xpcshell/head.js) | running event loop

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 871] onInstalled == onInstalled
*** LOG addons.xpi: Install of file:///Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/addons/test_langpack.xpi completed.

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 942] 6 == 6

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 943] 0 == 0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 944] onInstallEnded == onInstallEnded

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 62] true == true

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 73] 0 == 0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 76] [object Object] != null

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 77] 1.0 == 1.0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 78] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 79] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 80] true == true

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 81] true == true

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 82] x-testing == x-testing

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 83] true == true

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 84] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 90] 0 == 0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 109] 0 == 0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 845] onDisabling == onDisabling

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 846] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 849] false == false
*** LOG addons.xpi-utils: Updating add-on state

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 855] onDisabled == onDisabled

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 856] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 113] [object Object] != null

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 114] 1.0 == 1.0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 115] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 116] true == true

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 117] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 118] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 128] [object Object] != null

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 129] 1.0 == 1.0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 130] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 131] true == true

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 132] false == false
*** LOG addons.manager: shutdown
*** LOG addons.xpi: shutdown
*** LOG addons.xpi-utils: Updating add-on states
*** LOG addons.xpi-utils: shutdown
*** LOG addons.xpi-utils: Database closed

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [run_test_3 : 142] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [run_test_3 : 148] true == true
*** LOG addons.xpi: startup
*** LOG addons.xpi: checkForChanges
*** LOG addons.xpi-utils: Opening database
*** LOG addons.xpi: No changes found
*** LOG addons.xpi: Add-ons list is invalid, rebuilding

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [run_test_3 : 151] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [run_test_3 : 157] true == true

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 162] [object Object] != null

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 163] 1.0 == 1.0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 164] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 165] true == true

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 166] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 183] 0 == 0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 828] onEnabling == onEnabling

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 829] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 832] false == false
*** LOG addons.xpi-utils: Updating add-on state

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 838] onEnabled == onEnabled

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 839] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 187] [object Object] != null

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 188] 1.0 == 1.0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 189] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 190] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 191] true == true

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 192] true == true

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 193] x-testing == x-testing

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 197] [object Object] != null

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 198] 1.0 == 1.0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 199] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 200] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 201] true == true
*** LOG addons.manager: shutdown
*** LOG addons.xpi: shutdown
*** LOG addons.xpi-utils: Updating add-on states
*** LOG addons.xpi-utils: shutdown
*** LOG addons.xpi-utils: Database closed

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [run_test_5 : 211] false == false

TEST-UNEXPECTED-FAIL | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | x-testing == x-test - See following stack:
JS frame :: /Users/axelhecht/src/central/mozilla-central/testing/xpcshell/head.js :: do_throw :: line 451
JS frame :: /Users/axelhecht/src/central/mozilla-central/testing/xpcshell/head.js :: _do_check_eq :: line 545
JS frame :: /Users/axelhecht/src/central/mozilla-central/testing/xpcshell/head.js :: do_check_eq :: line 566
JS frame :: /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js :: run_test_5 :: line 214
JS frame :: /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js :: <TOP_LEVEL> :: line 203
JS frame :: resource://gre/modules/AddonManager.jsm :: safeCall :: line 78
JS frame :: resource://gre/modules/AddonManager.jsm :: <TOP_LEVEL> :: line 1634
JS frame :: resource://gre/modules/XPIProvider.jsm :: getAddonByID_getVisibleAddonForID :: line 3200
JS frame :: resource://gre/modules/XPIProvider.jsm -> resource://gre/modules/XPIProviderUtils.js :: <TOP_LEVEL> :: line 1363
JS frame :: resource://gre/modules/XPIProvider.jsm -> resource://gre/modules/XPIProviderUtils.js :: <TOP_LEVEL> :: line 177

TEST-INFO | (xpcshell/head.js) | exiting test

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [run_test_5 : 217] true == true
*** LOG addons.xpi: startup
*** LOG addons.xpi: checkForChanges
*** LOG addons.xpi: No changes found
*** LOG addons.xpi: Add-ons list is invalid, rebuilding
*** LOG addons.xpi-utils: Opening database

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [run_test_5 : 221] true == true

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [run_test_5 : 222] x-testing == x-testing

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 1266] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 1269] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js | [null : 1272] false == false
*** LOG addons.manager: shutdown
*** LOG addons.xpi: shutdown
*** LOG addons.xpi-utils: shutdown
*** LOG addons.xpi-utils: Opening database
*** LOG addons.xpi-utils: Database closed

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 226] [object Object] != null

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 227] 1.0 == 1.0

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 228] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 229] false == false

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 230] true == true

TEST-PASS | /Users/axelhecht/src/central/mozilla-central/obj-fx-debug/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_langpack.js | [null : 231] false == false
*** WARN addons.manager: Exception calling callback: [Exception... "AddonManager is not initialized"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: resource://gre/modules/AddonManager.jsm :: AMI_getAddonByID :: line 1620"  data: no]
WARNING: nsExceptionService ignoring thread destruction after shutdown: file /Users/axelhecht/src/central/mozilla-central/xpcom/base/nsExceptionService.cpp, line 166
WARNING: NS_ENSURE_TRUE(asyncCloseWasCalled) failed: file /Users/axelhecht/src/central/mozilla-central/storage/src/mozStorageConnection.cpp, line 890
WARNING: OOPDeinit() without successful OOPInit(): file /Users/axelhecht/src/central/mozilla-central/toolkit/crashreporter/nsExceptionHandler.cpp, line 2219
nsStringStats
 => mAllocCount:           6256
 => mReallocCount:          494
 => mFreeCount:            6256
 => mShareCount:           9250
 => mAdoptCount:            299
 => mAdoptFreeCount:        299


Any clues?

Comment 28

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Axel Hecht [:Pike] from comment #27)
> The crash reporter stuff doesn't seem to mention langpacks, is that
> intentional?

Yes. AFAIK, crash reports don't currently have any details on which locale the app is using. If we want to revisit that, it should be in a separate bug (THB, its not something I've thought about until now, though maybe Dave has).


> The chrome registry throws on getSelectedLocale, which mostly works
> allright.

Hmm, yes. It seems to work when waiting awhile before doing that check (by using do_timeout), so I presumably something in there is async. I don't know of any events, however - so you'll need to wait 10ms, check, if that fails, wait another 10ms, etc - for some reasonable maximum timeout.


> But I see a failure about the addons manager not being
> initialized,

Ignore this - it's an artifact of the test failing due to the getSelectedLocale() check.

Comment 29

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 658085 [details] [diff] [review]
tests for the patch

Review of attachment 658085 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/test/xpcshell/test_dictionary.js
@@ +139,5 @@
>  // Test that restarting doesn't accidentally re-enable
>  function run_test_3() {
>    shutdownManager();
> +  do_check_false(false); // XXX check chrome reg
> +  try {

Don't check getSelectedLocale() after shutdown - it can't be hit in the real world (only in tests). run_test_2 already covers this anyway, testing when the langpack is disabled.

Comment 30

[Axel Hecht [:Pike]]

Attachment: patch and tests

So, here's tests and patch in one, requesting review on this one.

I have one test, unregistered langpack on addon manager shutdown, which fails. I don't know why it does, by debugging looked like there's no bootstrap method called in that event, and thus no unregistering happens?
I made the test pass with an explicit throw.

https://tbpl.mozilla.org/?tree=Try&rev=fa77f2d39edc passed mochitests, not sure why win-opt didn't build, given that all the other passed I didn't bother to spend more try cycles on it.

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 662850 [details] [diff] [review]
patch and tests

Review of attachment 662850 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/test/xpcshell/test_dictionary.js
@@ +4,5 @@
>  
> +// This verifies that language packs can be used without restarts.
> +// There's commented out tests on whether the language pack shows up
> +// in crash reports or not, right now they don't, but if we decide
> +// to add that in the future, we know what to do.

Don't leave those in there on a "maybe" - just remove them. Its easy enough to add in the future them if we ever need them.

@@ +120,5 @@
>      do_check_false(b1.isActive);
> +    // check chrome reg that language pack is not registered
> +    try {
> +      // throw if still registered
> +      do_check_eq(chrome.getSelectedLocale('test-langpack'), 'x-test');

Don't mix quote styles - standard everywhere here is only double quotes.

@@ +148,5 @@
> +    // throw if still registered
> +    do_check_eq(chrome.getSelectedLocale('test-langpack'), 'x-test');
> +    do_throw('language pack must not be registered now');
> +  } catch (e) {
> +    do_check_true(Boolean('language pack is not registered'));

Whats up with this try/catch block, and others like it? To check it's not registered, just use do_check_neq

@@ +203,5 @@
>  // Tests that a restart shuts down and restarts the add-on
>  function run_test_5() {
>    shutdownManager();
> +  // check chrome reg that language pack is not registered.
> +  // XXXPike not sure why this is still around, though.

This is expected - we don't bother unregistering chrome.manifest files on shutdown, as there's no reason to (they're not persisted). The only time that's noticeable is in tests. So you should remove the check between shutdown and restart.

Interestingly, dictionaries *are* being unregistered on shutdown, but don't need to be - filed bug 793662.

Comment 32

[Axel Hecht [:Pike]]

(In reply to Blair McBride (:Unfocused) from comment #31)
> Comment on attachment 662850 [details] [diff] [review]
> patch and tests
> 
> Review of attachment 662850 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review, I'll address the other comments.

> 
> @@ +148,5 @@
> > +    // throw if still registered
> > +    do_check_eq(chrome.getSelectedLocale('test-langpack'), 'x-test');
> > +    do_throw('language pack must not be registered now');
> > +  } catch (e) {
> > +    do_check_true(Boolean('language pack is not registered'));
> 
> Whats up with this try/catch block, and others like it? To check it's not
> registered, just use do_check_neq

This one is a bit cumbersome, but intentional. The chrome registry throws an error if there's no registered locale for a package. Thus I make a passed test if that line fails, and a failed test if it doesn't.

I didn't find any shrink-wrapped tooling ala python's assertRaises, so I hand-coded something.

Comment 33

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Axel Hecht [:Pike] from comment #32)
> This one is a bit cumbersome, but intentional. The chrome registry throws an
> error if there's no registered locale for a package. Thus I make a passed
> test if that line fails, and a failed test if it doesn't.
> 
> I didn't find any shrink-wrapped tooling ala python's assertRaises, so I
> hand-coded something.

Ah!

Something like this would be nicer/more obvious/easier to read:

  let didThrow = false;
  try {
    chrome.getSelectedLocale("test-langpack");
  } catch (e) {
    didThrow = true;
  }
  do_check_true(didThrow);

Comment 34

[Axel Hecht [:Pike]]

Attachment: patch with passing tests

This patch passes tests, and I got optimistic, as you can see in the commit message.

I've factored out the code snippet you suggested into do_check_locale_not_registered, to make the tests more descriptive, removed the commented out stuff and the shutdown test, and converted the quotes.

Comment 35

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 664658 [details] [diff] [review]
patch with passing tests

Review of attachment 664658 [details] [diff] [review]:
-----------------------------------------------------------------

Good to go!

Comment 36

[Axel Hecht [:Pike]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/812d0ba83175

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/812d0ba83175

Comment 38

[Reşat SABIQ (Reshat)]

I'd appreciate any feedback on whether the fix for this enhancement request could be the cause for bug 701631 (which is easily reproducible if you have a few minutes of time to follow a few simple steps).

Thanks.

Comment 39

[Dave Townsend [:mossop]]

(In reply to Reşat SABIQ (Reshat) from comment #38)
> I'd appreciate any feedback on whether the fix for this enhancement request
> could be the cause for bug 701631 (which is easily reproducible if you have
> a few minutes of time to follow a few simple steps).
> 
> Thanks.

Seems unlikely, bug 701631 is reported as happening as early as Firefox 8, this bug was only fixed for Firefox 18.

Comment 40

[Scoobidiver (away)]

Backed out to fix bug 818468:
https://hg.mozilla.org/releases/mozilla-beta/rev/65ec01c62495
https://hg.mozilla.org/releases/mozilla-release/rev/4fe6923afe8a

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Backed out on Aurora too:
https://hg.mozilla.org/releases/mozilla-aurora/rev/08c0832fbb6e

Comment 42

[Gingerbread Man]

If this was backed out, why is it marked fixed?
Also see bug 1213781.

Comment 43

[Axel Hecht [:Pike]]

The restartlessness was only backed out on release branches, but since has made it to release, see https://hg.mozilla.org/mozilla-central/annotate/c281177594d6/toolkit/mozapps/extensions/XPIProvider.jsm#l827

Comment 44

[Gingerbread Man]

How peculiar. The tracking flags don't indicate anything of the sort, there's no mention in either the release notes or the developer release notes, and bug 865840 hasn't been fixed after all this time. Thank you for clearing that up.