44070 - [deployment]Match browser's default locale/region to OS's default

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P1)

Description

[Henrik Gemal]

Now that we got the region selection in the Create Profile the region 
selection should be a little bit smart.
The default region in the Region Selection should be taken from fx the default 
region on my Win32 machine. So if I have Locale = Danish in my Control Panel the 
default region should be danish.
I know currently that Danish isn't availble as a locale, but "English [GB]" is, 
so if you have "English [GB]" in your Win32 control panel "English [GB]" should 
be the default.

Comment 1

[Peter "jag" Annema]

Changed subject.
Old subject: Default region should not just be English

Changed severity to Enhancement.

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

over to i18n, who implemented the back end for this feature.  when they 
have a means of telling me how to reflect this in the FE, I'll do it. 

Comment 3

[nhottanscp]

Tao, could you look at this?

Comment 4

[tao]

There is a decision to make:

  Should system locale or client's localization version as the client's default
  locale?

We wo't be able to make a call, now. Accept the bug and mark it M20.

Comment 5

[Peter "jag" Annema]

Well, the client's localisation is a choice made after the OS' choice, and based 
upon different needs. Also, in a multi user OS environment, the user's choice of 
locale may differ from the SysAdmin's choice. Therefore I suggest that the 
default is based on the client and not the OS.

Comment 6

[timeless]

Bah. Windows has defaults both at admin and per user level.  You ask the os 
what the current default is.  The answer you get is a function of the current 
user's preferences [if the admin didn't tell the box to refuse changes].  There 
should never be a harm in starting w/ a choice based on the os's understanding 
of the user. The only issue is being able to retrieve a compatible language 
pack.

Comment 7

[Henrik Gemal]

RFE cleanup. RFE is already indicated by the Severity field...Sorry for the 
spam!

Comment 8

[tao]

*** Bug 97791 has been marked as a duplicate of this bug. ***

Comment 9

[tao]

Making 97791 a dup of this. TFV->1.0.

Requests from SUN and Redhat follows.
-------------------------------------

Yes, I have prepared -UILocale and -contentLocale option to
switch the language at startup, and will provide own startup
script for Solaris to specify proper option by looking $LANG.
Because Solaris has to support multi-user and multi-locale
environment in one binary, multiple language packs need to be
installed into the same location, but Mozilla needs to be
invoked in proper UI locale for user desktop.

It would be better that Mozilla has the language selection
for UI and content by $LANG when the language pack is installed.
This should be the default behavior. When users want to ignore it,
users can use -UILocale and -contentLocale option, or setting
preference. How about adding "System" or "Desktop" item into
language selection in preferece and set this by default? The option
will check the locale of user's desktop and apply it to Mozilla
if the lang pack is available.

katakai

Yung-Fong Tang wrote:

Good question, Katakai from Sun request the something before.
I believe we have a command line work around now.
Do we remember why we don't listen to LANG to decide that value? I
remember there are some kind of Chicek-and-egg problem there.

Is that true the current behavior is depend on a pref value (as cross
platform behavior).

Christopher Blizzard wrote:


Why is it that Mozilla doesn't support using LANG on unix?  I would
expect that if I have LANG set to something other than en_US that it
would choose another locale if the lang pack is installed.

--Chris

Comment 10

[Christopher Blizzard (:blizzard)]

Any luck with this so far?

Comment 11

[tao]

reassign to new owner ->dbragg

Comment 12

[tao]

Attachment: patch that auto-select locale provider based on OS locale.

Hi, Don: 

This patch contains my work of matching browser locale provider to OS locale.
When 
proper version of langpacks needed could not be located, the current locale
provider will be used.

Please apply the patch to your tree, play with it, and then work with UE
engineer to iron out the usability issues. 

Many Thanks,

Tao

Comment 13

[Jimmy Uen]

Update QA contact to [e-mail]

Comment 14

[tao]

This is really a bad user experience and deployment headache; especially for 
UNIX and Mac. We are the only application speaks different language on native
desktop --> p1 & major

Please comment if there is an objection.

Comment 15

[tao]

Better yet; vote for this bug:
http://bugzilla.mozilla.org/showvotes.cgi?voteon=44070.

Comment 16

[Hasbullah Pit]

For information, my locale (ms) does not available in windows :(

Comment 17

[dbragg]

We may be one of the only apps that speaks a different language but we'er also
of the only apps that is a little "system within a system".  It's the multiple
profile aspect that makes this a complex issue. Is there any other app out there
that has this multiple profile functionality?

For example if someone has an english "account" and they have more than one
profile what should be done if they set all their profiles to use german
language packs?  The Profile Manager will always be in english and it will
always come up because they have multiple profiles.  Even if we don't implement
this feature the Profile Manager will always come up in the default language of
Mozilla that was installed.

I've got this up and running and this is exactly what is happening.  I'm not
convinced that this feature solves much given the nature of our multiple profile
product.  Please enlighten me with scenarios how this should work in a multiple
profile scenario.

Comment 18

[tao]

Don - the issue here is not about user profile. It's about the expected behavior
on desktop with multi-lingual support and centralized installation in 
multi-lingual  environment. How does an English user like to see a non-English app
on his English only desktop? Assuming this user read English only? He probably does
not know what that is and what to do with it.

Comment 19

[dbragg]

I understand that aspect of the issue.  What you're describing is the single
profile case though.  I believe this issue is more complex.  I will discuss it
outside of this bug to cut down on clutter.

Comment 20

[dbragg]

Attachment: New, merged version of Tao's patch.

This patch has the changes hyatt made to collapse the all-*.rdf files into one.
 This was bug 109488.

Comment 21

[tao]

Attachment: match OS locale when no cmdline override; use profile locale after profile selection

a new patch that matches 2002-03-18 trunk files; also clean up un-necessary 
code.

Comment 22

[tao]

Hi, Don - would you please verify this patch and make up some langpack for test
purpose? Or you can apply it to 0.9.9 branch and use MLP contributed packs for
testing. If all goes well, please review it and get sr from Hyatt. thx!

Hi Blizzard - do you mind reviewing this patch as well:

 1.On start up (in nsAppRunner.cpp), we check if "intl.locale.matchOS" == true 
  (default value), If yes, get system locale from OS and set runtime locale to
   match the system's if there is no cmdline override.
 2. After the user profile is selected, we honor the profile locale choosen by
   users.

In chrome registry, I added one new api to turn setRuntimeProvider on to signal
SetProviderForPackages not to flush the provider change and turn the flag off
right away so it won't affect subsequent calls.

Comments?

Comment 23

[tao]

Attachment: fix a flaw in nsChromeRegistry::SetProviderForPackage()

Fix a flaw in previous patch

nsChromeRegistry::SetProviderForPackage():
...
...
-  if (!mBatchInstallFlushes)
+  if (!mBatchInstallFlushes && !mRuntimeProvider) {
     rv = remote->Flush();

// this "if" statement should be moved out of this block; see the new patch
+    if (mRuntimeProvider)
+      mRuntimeProvider = PR_FALSE;
+  }

Comment 24

[tao]

Both Sun and Redhat asked for this fix.-->[adt2]

Comment 25

[tao]

Both Sun and Redhat asked for this fix.-->[adt2]

Comment 26

[dbragg]

Windows test build has been delivered to QA.

Comment 27

[tao]

Attachment: patch for profileCreation code: don't set profile locale if the user does not choose one; use current locale instead.

patch for profileCreation code: don't set profile locale if the user does not
choose one; use current locale instead. Note that the current locale could be
the global locale, cmdline override, or detected system locale.

Comment 28

[Brendan Eich [:brendan]]

Who are the module owners who should r= these patches?

/be

Comment 29

[tao]

Brendan:

Good question. I looked at this page: http://www.mozilla.org/owners.html but 
wasn't sure if it is up-to-date. Lots of module owners/peers are no longer 
active. Anyway, my best guess:

  xpfe/bootstrap/nsAppRunner.cpp - browser? everyone?
  chromeregistry - david hyatt and ? (I saw you sr hyatt's patch;
  language pack concept - who can sr this?

Comment 30

[Brendan Eich [:brendan]]

tao, I'll sr= *after* appropriate owner-type people stamp r= -- hyatt should r=
the chrome registry patch, I propose ccarlen r= the profile change.

/be

Comment 31

[tao]

Comment on attachment 75627 [details] [diff] [review]
patch for profileCreation code: don't set profile locale if the user does not choose one; use current locale instead.

This patch won't work now since we always set profile locale now.

Comment 32

[tao]

Thanks to dbragg for verifying the patch. --> tao

Comment 33

[timeless]

+        printf("\n --nsProfile::CreateNewProfileWithLocales, old locale=(%s, 
%s)\n", 
+               ToNewCString(currentUILocaleName), 
ToNewCString(currentContentLocaleName));

look into PrintfCString

patches generally shouldn't include #if 0'd comments, and there should be a 
good reason for #ifdef DEBUG_me

Personally, I'm opposed to intl attacking bootstrap.  In almost all cases, you 
should be able to create your own commandline handler that doesn't live there.

Comment 34

[tao]

> patches generally shouldn't include #if 0'd comments, and there should be a 
> good reason for #ifdef DEBUG_me

You're right. My bad. I will take them out and submit a new patch.

> Personally, I'm opposed to intl attacking bootstrap. 
Locale is a fundamental part of globalized application. All messages, monetary
symbols, for example, $ and euro, collation, time format, etc., all tie to the
system locale. Would you suggest a better place for this code segment to live?

> In almost all cases, you
> should be able to create your own commandline handler that doesn't live there.
Command line switches are for human intervention. Our goal here is to add
linguistic intelligence to Mozilla so it speaks the proper language on desktop.

Comment 35

[tao]

Attachment: patch for profile: take out #if 0, #if defined, etc.

clean up http://bugzilla.mozilla.org/attachment.cgi?id=75627. This patch won't
be applied until we stop auto-set the profile locale.

Comment 36

[Conrad Carlen (not reading bugmail)]

Comment on attachment 76129 [details] [diff] [review]
patch for profile: take out #if 0, #if defined, etc.

This patch looks like the last one, which in comment #31 was said not to work,
but with the #if 0, #if defined stuff removed. What did you mean in that
comment?

Comment 37

[Alec Flett]

good lord! NONE of that code should be in xpfe/bootstrap.. this should be
initialized with command-line handlers, like timeless said. The argument about
how locale is an integral part of any application justifies the code's
existence, but not its location. this code should live in an i18n dll either:
1) use the standard command-line handler mechanism (nsICmdLineHandler) and/or
the app-startup mechanism (look at a file called nsIAppStartup-something)
2) have a single do_GetService()/Init() method which does all this work

the first option is much preferred over the 2nd.. the 2nd should only happen if
stuff has to be set up before the rest of the command line handlers.

Comment 38

[tao]

Comment on attachment 76129 [details] [diff] [review]
patch for profile: take out #if 0, #if defined, etc.

Obsolete this patch to avoid confusion. Will resurrect it
after we stop saving  auto-selection of providers 
to profile folder.

Comment 39

[tao]

>good lord! NONE of that code should be in xpfe/bootstrap..
Sounds like I am an idiot :-\

> this should be initialized with command-line handlers, like timeless said. 
Are you suggesting that we use commandline switch such as
"-UILocale"/"-ContentLocale" or something else?

> The argument about how locale is an integral part of any application justifies
> the code's existence, but not its location.
Well said.
> this code should live in an i18n dll either:
> 1) use the standard command-line handler mechanism (nsICmdLineHandler) and/or

What's your concern? The use of preference or the location of the code?
Are you in favor of cmdline switch over user preference?

> the app-startup mechanism (look at a file called nsIAppStartup-something)
> 2) have a single do_GetService()/Init() method which does all this work
> the first option is much preferred over the 2nd.. the 2nd should only happen 
> if stuff has to be set up before the rest of the command line handlers.

Alec - 

1. as I said in Comment #34, the intention of this is to dynamically 
   select Browser's UI language and region setting based on the desktop's locale
   instead of achieving the same goal using cmdline switch which relies on human
   intervention.
2. this auto-selection MUST take place after chromeRegistry initialized and 
   before any chrome url conversion happen. It also needs to know if valid
   cmdline switch "-UILocale" and "-contentLocale" have been used. If so, 
   honor them. 
3. adding such function into i18n dll would introduce a new chain of dependency 
   on libpref and chromeRegistry into i18n dll which lots of modules already
   depend on.
4. my first intuition is to do this within existing function,
   InstallGlobalLocale() which is called right after cmdLineArgs->Initialize(,).

Comments?

Comment 40

[Alec Flett]

sorry, I think none of the code, even the existing code, belongs in
xpfe/bootstrap... I just talked with tao on the phone, and we uncovered some odd
things. 

Here are my issues, that we'll table until after 1.0:

If we're concerned about dependencies, then maybe we need to rethink what we're
trying to accomplish... We're trying to set the global locale at a specific
place during startup. You've hit the two events on the head:
- after chrome registry initialization
- before any chrome url conversion

it sounds more like we need the chrome registry to notify this new code that it
has been initialized, and we need the initialization to honor the command line
if any was passed in.

To me that says:
1) a global service that watches for chrome registry initialization (isn't there
already an observer mechanism set up for this?)
2) this global service should ALSO implement the command-line handler, to handle
any command-lines and override the global one. command line handers should fire
before the chrome registry initializes (at least I would hope) so this shouldn't
be a problem

this code should not live in xpfe/bootstrap, because xpfe/bootstrap has too much
stuff in it already - you talk about dependencies, but you've just introduced
"locale" to xpfe/bootstrap/makefile.win

------

ok, that said, we're going to leave InstallGlobalLocale() in nsAppRunner.cpp for
1.0, and move it out afterwards. The reason for this is that there is some risk
that we're going to muck with the initialization order and cause other problems.
The problem is that InstallGlobalLocale's GetService() call to get the chrome
registry is more than likely the call that creates the chrome registry in the
first place. If we start rearranging code now, we might change the
initialization order, and we all saw what that did back when I reversed the
order of observer notifications :)

Now for the review of this existing patch...

Comment 41

[tao]

Hi, Alec - here is the bug to move InstallGlobalLocale() out of bootstrap:
http://bugzilla.mozilla.org/show_bug.cgi?id=133566. Thanks for the valuable 
suggestion.

Comment 42

[Alec Flett]

Comment on attachment 75445 [details] [diff] [review]
fix a flaw in nsChromeRegistry::SetProviderForPackage()


>+  /* don't assert the runtime change */
>+  void setRuntimeProvider(); 

This, as well as mRuntimeProvider, needs a better name - What's a "runtime
provider"? What is it providing? a locale?
maybe this should be an attribute:
attribute boolean hasRuntimeLocaleProvider;
(that doesn't sound right either, I'm sure you can come up with something
better than mine :))

> 
>-  if (!mBatchInstallFlushes)
>+  if (!mBatchInstallFlushes && !mRuntimeProvider)
>     rv = remote->Flush();

explain why this is important? 

>+
>+  if (mRuntimeProvider)
>+    mRuntimeProvider = PR_FALSE;
>+

no need for the if(), just set mRuntimeProvider = PR_FALSE


>+// match OS locale
>+static char kMatchOSLocalePref[] = "intl.locale.matchOS";
>+
>+nsresult
>+getCountry(PRUnichar *lc_name_unichar, PRUnichar **aCountry)
>+{
>+
>+  nsresult        result = NS_OK;
>+  nsAutoString  	category; category.AssignWithConversion("NSILOCALE_MESSAGES");

nope!
NS_NAMED_LITERAL_STRING(category, "NSILOCALE_MESSAGES");

>+
>+  nsAutoString	  lc_name;
>+  lc_name.Assign(lc_name_unichar);

does this need to be an nsAutoString? Why not just
nsDependentString lc_name(lc_name_unichar)

or even better, just make the first parameter be |const nsAString&
lc_name_unichar|

>+  // nsMemory::Free(lc_name_unichar);
>+
>+  PRInt32   dash = lc_name.FindCharInSet("-");

the "set" of one character is just the one character:
PRInt32 dash = lc_name.FindChar('-');

>+
>+static nsresult
>+getUILangCountry(PRUnichar** aUILang, PRUnichar** aCountry)

again, make out parameters nsAString&, you'll find them easier to manage

>+{
>+	nsresult	 result;
>+	// get a locale service 
>+	nsCOMPtr<nsILocaleService> localeService = do_GetService(NS_LOCALESERVICE_CONTRACTID, &result);
>+	NS_ASSERTION(NS_SUCCEEDED(result),"nsLocaleTest: get locale service failed");
>+
>+  result = localeService->GetLocaleComponentForUserAgent(aUILang);
>+  result = getCountry(*aUILang, aCountry);

then, here you can say
result = getCountry(nsDependentString(*aUILang), aCountry);

>+    // match os locale
>+    nsXPIDLString uiLang;
>+    nsXPIDLString country;
>+    if (matchOS) {
>+      // compute lang and region code only when needed!
>+      rv = getUILangCountry(getter_Copies(uiLang), getter_Copies(country));

then these can just be nsAutoStrings.

the overall approach is fine - this just makes us be a little more
heap-friendly.

Comment 43

[Alec Flett]

actually, this:

+    nsAutoString lang;
+    nsAutoString country;
+    PRInt32 count = 0;
+    count = lc_name.Left(lang, dash);
+    count = lc_name.Right(country, (lc_name.Length()-dash-1));
+    *aCountry = ToNewUnicode(country);

should be this:

aCountry = Substring(lc_name, dash, lc_name.Length()-dash-1);

and I'm not even sure of the point of "lang" above :)

Comment 44

[tao]

Attachment: clean up string manipulation in nsAppRunner.cpp; remove "if check" in nsChromeRegistry.cpp per alecf's suggestion.

Hi, Hyatt/Alecf: please review this new patch containing alecf's suggestion
about string manipulation.

Hi, Alec: the function setRuntimeProvider() is to temporarily disable data
source
assertion so we don't overwrite global chrome provider. It affects both skin
and 
locale providers since there is no 'if check' for locaele provider.

Comment 45

[Alec Flett]

Comment on attachment 76339 [details] [diff] [review]
clean up string manipulation in nsAppRunner.cpp; remove "if check" in nsChromeRegistry.cpp per alecf's suggestion.

oops, I just realized, what IS "category" for anyway? I don't see it used
anywhere! :)

other than that, and the API change we talked about, looks good.

Comment 46

[tao]

Attachment: same patch except setRuntimeProvider() becomes setRuntimeProvider(in boolean)

this patch is the same as the previous patch except the change of
setRuntimeProvider(void) to setRuntimeProvider(in boolean ) since alecf would
like
to preserve a room for the case that callers do want to explicitly turn off the

flag for such reason:

-- from alecf --

I could come up with one: say an embeddor running mozilla at a kioskwants to
allow users to install new chrome for their session, but doesn'texist
permenantly... however they want to be able to install their own chromeand make
it stick
alecflett (6:16:11 PM): they could temporarily turn off the flag if the user
ever tries to install their own chrome,
alecflett (6:16:19 PM): but keep the flag on when they install a system-chrome
provider
alecflett (6:16:54 PM): I still don't understand the objection to the boolean..
it doesn'thurt anything, and if we're never passing in PR_FALSE then its no
problem

Comment 47

[tao]

Attachment: same as the previous patch except taking useless 'category'

alecf is right; the variable category is useless -> take it out.

Comment 48

[tao]

Comment on attachment 76352 [details] [diff] [review]
same as the previous patch except taking useless 'category'

per alecf on attachment 76339 [details] [diff] [review], r=alecf

Comment 49

[David Hyatt]

Comment on attachment 76352 [details] [diff] [review]
same as the previous patch except taking useless 'category'

sr=hyatt

Comment 50

[Asa Dotzler [:asa]]

Comment on attachment 76352 [details] [diff] [review]
same as the previous patch except taking useless 'category'

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 51

[John Keiser (jkeiser)]

Patch was checked in tonight; also backed out, we suspect it is the cause of a
startup regression.  Update when we find out for sure.

Comment 52

[John Keiser (jkeiser)]

OK, patch remains backed out.  It caused a 33% startup time increase.  No one
really knows why, we just guessed from the fact that AppRunner was touched.

Comment 53

[tao]

It might be pref initialization. I will take a look.

Comment 54

[Asa Dotzler [:asa]]

Comment on attachment 76352 [details] [diff] [review]
same as the previous patch except taking useless 'category'

removing approval until this can be done without Ts regression.

Comment 55

[tao]

It turns out that selecting locale is an expensive operation, when 
intl.locale.matchOS= true:

00001.089:   MATCH_OS...
00001.092:   ...MATCH_OS
00001.093:   MATCH_UI...
00001.093:    MATCH_GET_UI...
00001.099:    ...MATCH_GET_UI
00004.377:   ...MATCH_UI
00004.377:   MATCH_REGION...
00005.131:   ...MATCH_REGION

while when intl.locale.matchOS= false:

00001.053:   MATCH_OS...
00001.053:   ...MATCH_OS
00001.053:   MATCH_UI...
00001.053:   ...MATCH_UI
00001.053:   MATCH_REGION...
00001.053:   ...MATCH_REGION

I am recommiting this patch w/ this pref default to "false" and let system admins
want this feature to tur it on. The overhead is the same as the cmdline switch.

Comment 56

[tao]

OK, the startup regression goes away after I set 'intl.locale.matchOS' to 'false'.
The following is for the release note:
--------------------------------------

new pref: instroduced by http://bugzilla.mozilla.org/show_bug.cgi?id=44070
[deployment]Match browser's default locale/region to OS's default.

all.js: pref("intl.locale.matchOS",                 false);

When its value is 'true', browser will inquire the system's locale and set its
UI language and region content to match it. For example, when we install a 
Japanese Mach V browser client on an English (US) system, on start up, the client 
will use English as its UI language and US as its region locale until a user 
profile is selected. If a (pair) of locale is defined in the selected profile, 
browser will honor the profile locale. In addition, when cmdline switches,
'-UILocale' and/or 'contentLocale', present, the cmdline switch take precedence.

Note that selecting locale is an expensive operation, therefore, when this
pref is "true", there will be a roughly 30% start up performance drag.

Comment 57

[Brendan Eich [:brendan]]

Where is the expense, can you profile (jprof or better)?  What's it doing,
factoring primes?  Half a :-)

/be

Comment 58

[Jatin Billimoria]

Tao, if you feel this is worthy of being release noted, please submit it to Moz
1.0 relnote tracking bug: 

Bug 133795

Include this bug number, a brief explanation of the problem, and any workarounds.

Comment 59

[Kasumi]

tested on 2002-04-24-08-1.0.0
OS = Win XP Pro. JA

1. User has Admin authority
2. Change language Japanese to English(GB)
3. Install 2002-04-24-08-1.0.0
4. Create a new user frofile and use this to launch
5. Edit/Preferences/Appearance/Languages/Content  

Still default Languages/Content are English(US)/US Region.

Comment 60

[Alec Flett]

this was turned off by default, I think. You need to set that pref that tao
listed above.

Comment 61

[tao]

alec is right; please read my comment in
http://bugzilla.mozilla.org/show_bug.cgi?id=44070#c56.

Comment 62

[Kasumi]

tested 2002-04-19-08-1.0.0-JA
OS = Win 98 English

Change value the following.
all.js: pref("intl.locale.matchOS",                 true);
Still UI is Japanese until I select profile.

Comment 63

[tao]

would you please look at the "Pref dialog | Appearance | Language" UI to see if
the  English US UI language pack is installed at all? If not, what you are
seeing is normal; otherwise, let me know.

Comment 64

[Kasumi]

English（US）language pack is not installed.
I am seeing Japanese.

Comment 65

[tao]

You can install langenus.xpi and regus.xpi for the same date the Ja build base
off to test this feature.

Comment 66

[Andrea]

On 1.0 rc1 win32 with Italian lp installed I can see everything working fine
(Italian Profile Manager).

Comment 67

[Kasumi]

tested on 2002-07-29-08-1.0 JA using corresponding langenus.xpi and regus.xpi.
Win XP US SP1 Beta1
Verified

Comment 68

[Evan Yan]

should we re-consider enable intl.locale.matchOS as default?

The performance impact mentioned in comment #51 is five years ago. And I have tried enabling intl.locale.matchOS on our tinderbox for Solaris (http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox-Ports), no performance impact observed.

It's important because currently every OS distributions have to hold patch themselves, in order to have Firefox/Thunderbird can be launched in correct locale.