399153 - Software update should support unicode strings for the UI

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect, P4)

Description

[Axel Hecht [:Pike]]

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.

Comment 1

[Reed Loden [:reed]]

This blocks blocker bug 405871.

Comment 2

[Mike Connor [:mconnor]]

Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.

Comment 3

[(no longer active)]

Attachment: 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.

Comment 4

[Robert Strong (they/them - no direct email)]

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.

Comment 5

[(no longer active)]

Hmmm, so I guess I will need to pull comm-central anyway and patch them up.  :-)

Comment 6

[Robert Strong (they/them - no direct email)]

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.

Comment 7

[(no longer active)]

Yeah, that would be great if you can handle it.  Thanks!

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: comm-central patch

Comment 9

[Robert Strong (they/them - no direct email)]

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 10

[Robert Strong (they/them - no direct email)]

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 11

[Robert Strong (they/them - no direct email)]

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/

Comment 12

[Robert Strong (they/them - no direct email)]

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.

Comment 13

[Robert Strong (they/them - no direct email)]

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.

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: patch - alternate approach

Hey Ehsan, what do you think of this alternate approach?

Comment 15

[Robert Strong (they/them - no direct email)]

Attachment: patch - alternate approach rev2

Comment 16

[Robert Strong (they/them - no direct email)]

Attachment: comm-central patch rev2

Comment 17

[(no longer active)]

Comment on attachment 355735 [details] [diff] [review]
patch - alternate approach rev2

Looks very good.  Thanks for the patch!

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 355735 [details] [diff] [review]
patch - alternate approach rev2

a191=beltzner

Comment 19

[Robert Strong (they/them - no direct email)]

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

Comment 20

[Robert Strong (they/them - no direct email)]

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

Comment 21

[Robert Strong (they/them - no direct email)]

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.

Comment 22

[Robert Strong (they/them - no direct email)]

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

Comment 23

[Tony Chung [:tchung]]

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.

Comment 24

[Robert Strong (they/them - no direct email)]

(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.

Comment 25

[Tony Chung [:tchung]]

Attachment: 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.

Comment 26

[Robert Strong (they/them - no direct email)]

Hey Tony, those types of errors are almost always due to the locale files themselves and is definitely not due to this fix.

Comment 27

[Tony Chung [:tchung]]

yeah thought so.  okay i'll track this issue in a new bug.  thanks rob.

Comment 28

[(no longer active)]

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.

Comment 29

[Tony Chung [:tchung]]

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.