2.07 KB, patch
|Details | Diff | Splinter Review|
7.99 KB, patch
Benjamin Smedberg: review+
|Details | Diff | Splinter Review|
4.62 KB, patch
|Details | Diff | Splinter Review|
650.69 KB, image/png
To do my dirty deeds, it'd be good if we could drop needing a charset for updater, so that we can actually localizer that UI in all languages. See http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/90322a3043958f91/3c6e83c1b059a999?lnk=raot#3c6e83c1b059a999 for reference.
This blocks blocker bug 405871.
Priority: -- → P3
Target Milestone: --- → Firefox 3 M11
Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.
Created attachment 352361 [details] [diff] [review] Patch (v1) This bug is Windows-specific, right? As far as I could tell, there is nothing non-Unicode in the Windows progress UI implementation. _UNICODE and UNICODE are defined on the command line, so the Unicode APIs are used. This patch only makes sure that the updater.ini file is generated in UTF-16LE on Windows. I tested this by sticking a |ShowProgressUI(); return 0;| in NS_main in updater.cpp, and it can successfully show a Unicode Persian string. With this patch applied on top of the one in bug 305039, we wouldn't need toolkit/locales/en-US/installer/windows/charset.mk any more, so this patch drops that as well.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #352361 - Flags: review?(robert.bugzilla)
Comment on attachment 352361 [details] [diff] [review] Patch (v1) With the removal of charset.mk Seamonkey, Thunderbird, and Sunbird will all need to be updated at the same time.
Hmmm, so I guess I will need to pull comm-central anyway and patch them up. :-)
I can do it if it is a pain for you to do so... the patches will need to be reviewed by a reviewer for each app as well and I've got a comm-central repo. I'm going to focus on bug 305039 first anyways.
Yeah, that would be great if you can handle it. Thanks!
Created attachment 355680 [details] [diff] [review] comm-central patch
Requesting blocking since all of the required patches are attached to this bug and I'd like to get pre-approval to land this along with the Unicode installer patches
Comment on attachment 355680 [details] [diff] [review] comm-central patch btw: this patch requires the utf16-le-bom.bin from the patch in bug 305039
Comment on attachment 352361 [details] [diff] [review] Patch (v1) The locale is stored in the updater.ini and it appears that nsIINIParser.getString is failing when the file is UTF-16LE Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIINIParser.getString]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: file:///C:/moz/tmp-mozilla-central/ff-opt-static/dist/bin/components/nsUpdateService.js :: getLocale :: line 549" data: no] >diff --git a/browser/locales/Makefile.in b/browser/locales/Makefile.in >--- a/browser/locales/Makefile.in >+++ b/browser/locales/Makefile.in >... >@@ -349,7 +348,8 @@ ifeq ($(OS_ARCH),WINNT) > ifeq ($(OS_ARCH),WINNT) > cat $< $(srcdir)/updater_append.ini $(srcdir)/../installer/windows/nsis/updater_append.ini | \ > sed -e "s/%AB_CD%/$(AB_CD)/" | \ >- iconv -f UTF-8 -t $(WIN_INSTALLER_CHARSET) > $(FINAL_TARGET)/updater.ini >+ iconv -f UTF-8 -t UTF-16LE | \ >+ cat $(srcdir)/../../toolkit/mozapps/installer/windows/nsis/utf16-le-bom.bin - > $(FINAL_TARGET)/updater.ini Please use the following since the rest of this file does $(topsrcdir)/toolkit/mozapps/installer/windows/nsis/
Attachment #352361 - Flags: review?(robert.bugzilla) → review-
Comment on attachment 355680 [details] [diff] [review] comm-central patch I'm going to hold off on the comm-central reviews until the ini parser issue is resolved.
One way to resolve this... I believe it should be possible to do something similar to what is done for Linux and Mac OS X for reading the updater UI strings and then use MultiByteToWideChar to convert the string from UTF-8 to UTF-16LE.
Created attachment 355734 [details] [diff] [review] patch - alternate approach Hey Ehsan, what do you think of this alternate approach?
Created attachment 355735 [details] [diff] [review] patch - alternate approach rev2
Attachment #355735 - Flags: review?(ehsan.akhgari)
Created attachment 355736 [details] [diff] [review] comm-central patch rev2
Attachment #355680 - Attachment is obsolete: true
Comment on attachment 355735 [details] [diff] [review] patch - alternate approach rev2 Looks very good. Thanks for the patch!
Attachment #355736 - Flags: review?(ctalbert)
Attachment #355736 - Flags: review?(bugzilla)
Attachment #355736 - Flags: review?(philringnalda)
Assignee: ehsan.akhgari → robert.bugzilla
Comment on attachment 355735 [details] [diff] [review] patch - alternate approach rev2 a191=beltzner
Attachment #355735 - Flags: approval1.9.1+
Attachment #355736 - Flags: review?(philringnalda) → review+
Note: I am going to leave charset.mk and the associated include statements in the Makefiles when I check this in since they are still used by the installer. I'll remove them when I check in bug 305039
Axel, it looks like the following locales don't have a translated updater.ini and should be able to translate the updater.ini after this bug is fixed ml as gu-IN si bn-IN hi-IN ka mr kn pa-IN te
Pushed to mozilla-central (everything but the removal of the en-US charset.mk which I will remove from mozilla-central in bug 305039). http://hg.mozilla.org/mozilla-central/rev/177e11abd9d9 I'll push to mozilla-1.9.1 and comm-central after one cycle. I won't be removing the en-US charset.mk since that would be a string change.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Pushed to mozilla-1.9.1 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/881fdb2a350f Pushed to comm-central http://hg.mozilla.org/comm-central/rev/05d7c1308855
Depends on: 473417
Verified fix on Mozilla/5.0 (Windows; U; Windows NT 6.1; zh-CN; rv:1.9.1b3pre) Gecko/20090211 Shiretoko/3.1b3pre, Mozilla/5.0 (Windows; U; Windows NT 6.1; pa-IN; rv:1.9.1b3pre) Gecko/20090211 Shiretoko/3.1b3pre. On m-c, it looks like the fix is there also. However, firefox fails to startup after installing (for unicode builds it seems). I'll track down a regression and see if this bug is related, and comment here later.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
(In reply to comment #23) >... > On m-c, it looks like the fix is there also. However, firefox fails to startup > after installing (for unicode builds it seems). I'll track down a regression > and see if this bug is related, and comment here later. When you say "for unicode builds it seems" what specifically do you mean? All builds use the same code and this will help understand what might be failing.
Created attachment 362155 [details] Broken startup still diagnosing the problem. but here's what i have so far: on 1.9.1branch-l10n --- ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-1.9.1/: locales like zh-CN, ja, pa-IN - installs and opens firefox in their language correctly with a new profile. CHECK. on Mozilla-central-l10n --- ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n/: locales like zh-CN and ja will install in their language, but will not fire up firefox. instead, you are thrown with this attached error. FAIL. - However, if you opened up fr build, firefox will open correctly. This could be an entirely different bug than the fix here, but i'm still working on a regression range.
Hey Tony, those types of errors are almost always due to the locale files themselves and is definitely not due to this fix.
yeah thought so. okay i'll track this issue in a new bug. thanks rob.
I don't think this is worth filing a new bug, since it's caused by untranslated strings. You can see the dashboard <http://l10n.mozilla.org/dashboard/> for any locale to see if it's green before testing a build.
Yeah, we had a separate email thread and someone pointed me to the dashboard page. Looks like a known issue for trunk, so the subject is now moot. Thanks for clarifying.
You need to log in before you can comment on or make changes to this bug.