Closed Bug 677092 Opened 13 years ago Closed 12 years ago

Make language packs restartless by default

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla21
Tracking Flags:
Tracking Status
firefox18 --- disabled
firefox19 --- disabled
firefox20 --- disabled

People

(Reporter: Pike, Assigned: Pike)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

patch with passing tests
25.52 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review
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.
Depends on: 564667
Changing UI locale on the fly? That would be a nice touch. :-)
(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)
Separate, but already possible: so once you have restartless langpacks, locale switcher should be able to use them!
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.
Bug 591780 is about spellcheck dictionaries, not langpacks. Sorry!
No longer depends on: 591780
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.
Depends on: 591780
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. :-)
hmm, does it mean that we can retranslate XUL on fly?
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.
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.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
Target Milestone: --- → mozilla14
Status: RESOLVED → VERIFIED
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.
Status: VERIFIED → REOPENED
Resolution: WORKSFORME → ---
(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.)
Status: REOPENED → UNCONFIRMED
Ever confirmed: false
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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.
Apologies.
Not really depending on dictionary code, but it helped. I've got a local patch in the works, so taking.
Assignee: nobody → l10n
No longer depends on: 591780
Target Milestone: mozilla14 → ---
Attached patch make langpacks restartless (obsolete) — — Splinter Review
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?
Attachment #651494 - Flags: feedback?(dtownsend+bugmail)
... 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
(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?
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.
(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.
(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 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).
Attachment #651494 - Flags: feedback?(dtownsend+bugmail) → feedback+
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.
Attachment #651494 - Attachment is obsolete: true
Attachment #656392 - Flags: review?(bmcbride)
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.
Attachment #656392 - Flags: review?(bmcbride) → feedback+
Status: NEW → ASSIGNED
Attached patch tests for the patch (obsolete) — — Splinter Review
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 langpack-x-testing@tests.mozilla.org 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?
Attachment #658085 - Flags: feedback?(bmcbride)
(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 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.
Attachment #658085 - Flags: feedback?(bmcbride) → feedback+
Attached patch patch and tests (obsolete) — — Splinter Review
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.
Attachment #656392 - Attachment is obsolete: true
Attachment #658085 - Attachment is obsolete: true
Attachment #662850 - Flags: review?(bmcbride)
Blocks: 186596
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.
Attachment #662850 - Flags: review?(bmcbride) → review-
(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.
(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);
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.
Attachment #662850 - Attachment is obsolete: true
Attachment #664658 - Flags: review?(bmcbride)
Comment on attachment 664658 [details] [diff] [review] patch with passing tests Review of attachment 664658 [details] [diff] [review]: ----------------------------------------------------------------- Good to go!
Attachment #664658 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/812d0ba83175
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 818468
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.
(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.
Target Milestone: mozilla18 → mozilla21
If this was backed out, why is it marked fixed? Also see bug 1213781.
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
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.
Depends on: 1236204
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: