Software update should support unicode strings for the UI

VERIFIED FIXED in mozilla1.9beta3

Status

()

Toolkit
Application Update
P4
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Pike, Assigned: rstrong)

Tracking

({intl, verified1.9.1})

unspecified
mozilla1.9beta3
intl, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
blocking1.9 -
wanted1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.

Updated

11 years ago
Blocks: 405871
This blocks blocker bug 405871.
Flags: blocking-firefox3?

Updated

11 years ago
Priority: -- → P3
Target Milestone: --- → Firefox 3 M11

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+

Updated

10 years ago
Priority: P3 → P4
Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Product: Firefox → Toolkit

Comment 3

10 years ago
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.

Comment 5

10 years ago
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.

Comment 7

10 years ago
Yeah, that would be great if you can handle it.  Thanks!
Created attachment 355680 [details] [diff] [review]
comm-central patch
Attachment #355680 - Flags: review?(bugzilla)
Attachment #355680 - Flags: review?(philringnalda)
Attachment #355680 - Flags: review?(ctalbert)
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
Flags: blocking1.9.1?
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.
Attachment #355680 - Flags: review?(philringnalda)
Attachment #355680 - Flags: review?(ctalbert)
Attachment #355680 - Flags: review?(bugzilla)
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?
Attachment #355734 - Flags: review?(ehsan.akhgari)
Created attachment 355735 [details] [diff] [review]
patch - alternate approach rev2
Attachment #355734 - Attachment is obsolete: true
Attachment #355735 - Flags: review?(benjamin)
Attachment #355734 - Flags: review?(ehsan.akhgari)
Attachment #355735 - Flags: review?(ehsan.akhgari)
Created attachment 355736 [details] [diff] [review]
comm-central patch rev2
Attachment #355680 - Attachment is obsolete: true

Updated

9 years ago
Attachment #355735 - Flags: review?(ehsan.akhgari) → review+

Comment 17

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

Looks very good.  Thanks for the patch!

Updated

9 years ago
Attachment #355735 - Flags: review?(benjamin) → review+
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+
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-

Updated

9 years ago
Attachment #355736 - Flags: review?(bugzilla) → review+
Attachment #355736 - Flags: review?(philringnalda) → review+

Updated

9 years ago
Attachment #355736 - Flags: review?(ctalbert) → 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
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.

Comment 28

9 years ago
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.