[Linux] Supports the LANGUAGE environment variable....

[Linux] Supports the LANGUAGE environment variable.

This CL adds the support for the LANGUAGE environment variable, which is supported by gettext based applications for specifying a priority list of user prefered locales for UI messages translation.

Unlike gettext based applications, which support using different locales for messages translation and other locale categories, like LC_CTYPE, LC_COLLATE, LC_TIME, etc., chromium supports only one application locale for all localization operations.
This CL adds the support of specifying the application locale by LANGUAGE env variable, but doesn't make chromium to support above mentioned behavior of gettext based applications.

BUG=21080
: chromium doesn't honor locale fallbacks
TEST=Launch chrome with LANGUAGE=br:fr_FR:fr, French locale shall be used by chrome, as br is not supported by chrome yet.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27608

[Evan Martin, 2009-09-24 15:20:32]

What about LANG?  (I find this stuff really confusing.)

We had a user request being able to set LC_NUMERIC or some such.  I don't know
how important it is, but maybe it would be worth considering how hard it is.

http://codereview.chromium.org/236001/diff/1/2
File app/l10n_util.cc (right):

http://codereview.chromium.org/236001/diff/1/2#newcode427
Line 427: // See l10n_util_mac for that implementation.
Since we have an l10n_util_mac, maybe the function above belongs in an
l10n_util_posix?

http://codereview.chromium.org/236001/diff/1/2#newcode463
Line 463: candidates.push_back(system_locale);
In the Windows branch, we always put the system_locale in the candidates list. 
Do we want to do that here?

Consider:
LANG=invalid_lang ./chrome

http://codereview.chromium.org/236001/diff/1/3
File app/l10n_util_unittest.cc (right):

http://codereview.chromium.org/236001/diff/1/3#newcode161
Line 161: ::unsetenv("LANGUAGE");
Do you need to unset the icu default locale too?

[tony, 2009-09-24 18:38:40]

http://codereview.chromium.org/236001/diff/1/2
File app/l10n_util.cc (right):

http://codereview.chromium.org/236001/diff/1/2#newcode390
Line 390: std::vector<std::string> langs;
Can we just copy the code exactly from gettext so we get the exact same logic? 
I'm worried there are some edge cases that we'll differ.  We would need to put
the associated code in base/third_party/gettext.

http://codereview.chromium.org/236001/diff/1/2#newcode457
Line 457: const char* env_language = ::getenv("LANGUAGE");
Nit: GetEnvVar in base/sys_info.h

[James Su, 2009-09-25 01:39:21]

Thanks for your feedback, please see my comments below.
The LC_*/LANG variables are still handled in ICU's icu::Locale::getDefault().
I'll update this CL asap to fix a security issue.

James Su

http://codereview.chromium.org/236001/diff/1/2
File app/l10n_util.cc (right):

http://codereview.chromium.org/236001/diff/1/2#newcode390
Line 390: std::vector<std::string> langs;
On 2009/09/24 18:38:40, tony wrote:
> Can we just copy the code exactly from gettext so we get the exact same logic?

> I'm worried there are some edge cases that we'll differ.  We would need to put
> the associated code in base/third_party/gettext.

I don't think we can copy gettext's source code, which is a LGPL/GPL software.
And because gettext supports more complicated features than us, such as @quot
and @boldquot variations and multiple categories, it's not that easy for us to
copy its behavior exactly.

http://codereview.chromium.org/236001/diff/1/2#newcode427
Line 427: // See l10n_util_mac for that implementation.
On 2009/09/24 15:20:32, Evan Martin wrote:
> Since we have an l10n_util_mac, maybe the function above belongs in an
> l10n_util_posix?

This is a Linux specific feature, as LANGUAGE is actually a GNU extension. It
might not be suitable for l10n_util_posix.
And if we want to split the linux specific code into l10n_util_linux, we may
need to expose above internal functions publicly or duplicate them in
l10n_util_linux.

http://codereview.chromium.org/236001/diff/1/2#newcode457
Line 457: const char* env_language = ::getenv("LANGUAGE");
On 2009/09/24 18:38:40, tony wrote:
> Nit: GetEnvVar in base/sys_info.h

GetEnvVar accepts and returns wide string. I'd prefer to use getenv directly
here to make the code simple.

http://codereview.chromium.org/236001/diff/1/2#newcode463
Line 463: candidates.push_back(system_locale);
On 2009/09/24 15:20:32, Evan Martin wrote:
> In the Windows branch, we always put the system_locale in the candidates list.

> Do we want to do that here?
This is exactly the old logic of Windows branch. If none of lang_arg and
pref_locale is valid, then fallback to system locale.
But for Linux, gettext's behavior is honored here: if LANGUAGE is set, LC_*/LANG
will be ignored and the untranslated msgid will be returned if none of them
items  of LANGUAGE is valid.

> 
> Consider:
> LANG=invalid_lang ./chrome

[James Su, 2009-09-25 01:51:07]

On 2009/09/24 15:20:32, Evan Martin wrote:
> What about LANG?  (I find this stuff really confusing.)
> 
> We had a user request being able to set LC_NUMERIC or some such.  I don't know
> how important it is, but maybe it would be worth considering how hard it is.
If we know where does chrome use the i18n/l10n related features, eg.
numeric/monetary/time/address/telephone/name formatting, and string sorting,
then it'll be easy for us to evaluate the importance and difficulty for
supporting such feature.

> 
> http://codereview.chromium.org/236001/diff/1/2
> File app/l10n_util.cc (right):
> 
> http://codereview.chromium.org/236001/diff/1/2#newcode427
> Line 427: // See l10n_util_mac for that implementation.
> Since we have an l10n_util_mac, maybe the function above belongs in an
> l10n_util_posix?
> 
> http://codereview.chromium.org/236001/diff/1/2#newcode463
> Line 463: candidates.push_back(system_locale);
> In the Windows branch, we always put the system_locale in the candidates list.

> Do we want to do that here?
> 
> Consider:
> LANG=invalid_lang ./chrome
> 
> http://codereview.chromium.org/236001/diff/1/3
> File app/l10n_util_unittest.cc (right):
> 
> http://codereview.chromium.org/236001/diff/1/3#newcode161
> Line 161: ::unsetenv("LANGUAGE");
> Do you need to unset the icu default locale too?

[Evan Martin, 2009-09-25 02:03:08]

Thanks for your thoughtful comments.
LGTM

[James Su, 2009-09-25 02:39:39]

I just updated the CL to fix a potential security issue.

Regards
James Su

On 2009/09/25 02:03:08, Evan Martin wrote:
> Thanks for your thoughtful comments.
> LGTM

[tony, 2009-09-25 17:05:29]

http://codereview.chromium.org/236001/diff/5001/5002
File app/l10n_util.cc (right):

http://codereview.chromium.org/236001/diff/5001/5002#newcode400
Line 400: if (i->find_first_of(FilePath::kSeparators) != std::string::npos)
Can you add some tests cases for this?

http://codereview.chromium.org/236001/diff/5001/5003
File app/l10n_util_unittest.cc (right):

http://codereview.chromium.org/236001/diff/5001/5003#newcode157
Line 157: EXPECT_EQ("en-US", l10n_util::GetApplicationLocale(L""));
Why doesn't this fallback to the ICU default of fr?  Also, do you need to reset
this?

[jungshik at Google, 2009-09-25 18:04:20]

On 2009/09/25 01:51:07, James Su wrote:
> On 2009/09/24 15:20:32, Evan Martin wrote:
> > What about LANG?  (I find this stuff really confusing.)
> > 
> > We had a user request being able to set LC_NUMERIC or some such.  I don't
know
> > how important it is, but maybe it would be worth considering how hard it is.
> If we know where does chrome use the i18n/l10n related features, eg.
> numeric/monetary/time/address/telephone/name formatting, and string sorting,
> then it'll be easy for us to evaluate the importance and difficulty for
> supporting such feature.

On Windows, we made a decision not to honor the individual locale categories and
just use a single locale across categories. And, our calls to ICU APIs for
formatting/sorting/etc do not specify the locale, which means ICU apis just use,
for those operations, the default locale set early on in each process
(l10n_util). 

On Linux, it's simpler than on Windows. We can keep separate locales for each
category (and cache the resolved locales) and use them when calling ICU apis. 
Whether we really want to do that is a separate question. 

> > 
> > http://codereview.chromium.org/236001/diff/1/2
> > File app/l10n_util.cc (right):
> > 
> > http://codereview.chromium.org/236001/diff/1/2#newcode427
> > Line 427: // See l10n_util_mac for that implementation.
> > Since we have an l10n_util_mac, maybe the function above belongs in an
> > l10n_util_posix?
> > 
> > http://codereview.chromium.org/236001/diff/1/2#newcode463
> > Line 463: candidates.push_back(system_locale);
> > In the Windows branch, we always put the system_locale in the candidates
list.
> 
> > Do we want to do that here?
> > 
> > Consider:
> > LANG=invalid_lang ./chrome
> > 
> > http://codereview.chromium.org/236001/diff/1/3
> > File app/l10n_util_unittest.cc (right):
> > 
> > http://codereview.chromium.org/236001/diff/1/3#newcode161
> > Line 161: ::unsetenv("LANGUAGE");
> > Do you need to unset the icu default locale too?

[jungshik at Google, 2009-09-25 18:04:48]

thank you for the cl.

http://codereview.chromium.org/236001/diff/5001/5003
File app/l10n_util_unittest.cc (right):

http://codereview.chromium.org/236001/diff/5001/5003#newcode157
Line 157: EXPECT_EQ("en-US", l10n_util::GetApplicationLocale(L""));
On 2009/09/25 17:05:29, tony wrote:
> Why doesn't this fallback to the ICU default of fr?  Also, do you need to
reset
> this?

I have the same question :-)

[jungshik at Google, 2009-09-25 18:36:05]

http://codereview.chromium.org/236001/diff/5001/5003
File app/l10n_util_unittest.cc (right):

http://codereview.chromium.org/236001/diff/5001/5003#newcode158
Line 158: 
In addition to the above, the actual case mentioned in the bug (Breton at the
beginning of the list) may be useful. It's less robust than using 'xx', 'yy'
because one day we may support that language (although not so likely). On the
other hand, it's a real-life example.

[James Su, 2009-09-26 01:05:37]

Thanks a lot for your feedback. CL updated.

James Su

http://codereview.chromium.org/236001/diff/5001/5002
File app/l10n_util.cc (right):

http://codereview.chromium.org/236001/diff/5001/5002#newcode400
Line 400: if (i->find_first_of(FilePath::kSeparators) != std::string::npos)
On 2009/09/25 17:05:29, tony wrote:
> Can you add some tests cases for this?

Test case added. I just found that this check is actually not necessary, as
CheckAndResolveLocale() guarantees it.

http://codereview.chromium.org/236001/diff/5001/5003
File app/l10n_util_unittest.cc (right):

http://codereview.chromium.org/236001/diff/5001/5003#newcode157
Line 157: EXPECT_EQ("en-US", l10n_util::GetApplicationLocale(L""));
On 2009/09/25 17:05:29, tony wrote:
> Why doesn't this fallback to the ICU default of fr?  Also, do you need to
reset
> this?

According to gettext's behavior, if LANGUAGE is set and none of the locales
specified by it is available, the untranslated msgid will be used directly. In
this case LC_*/LANG will just be ignored.

http://codereview.chromium.org/236001/diff/5001/5003#newcode158
Line 158: 
In real life, 'xx' or 'yy' may exist. But it's ok to use them in this test, as
only the locales listed in above filenames[] array are valid for this test.

On 2009/09/25 18:36:05, Jungshik Shin wrote:
> In addition to the above, the actual case mentioned in the bug (Breton at the
> beginning of the list) may be useful. It's less robust than using 'xx', 'yy'
> because one day we may support that language (although not so likely). On the
> other hand, it's a real-life example.

[tony, 2009-09-28 17:38:46]

LGTM

http://codereview.chromium.org/236001/diff/11001/11003
File app/l10n_util_unittest.cc (right):

http://codereview.chromium.org/236001/diff/11001/11003#newcode164
Line 164: ::unsetenv("LANGUAGE");
Nit: We should probably cache the environment variable and reset it, but I don't
feel strongly about it.

[jungshik at Google, 2009-09-28 18:37:33]

lgtm with the brief comment about gettext's behavior added.

http://codereview.chromium.org/236001/diff/5001/5003
File app/l10n_util_unittest.cc (right):

http://codereview.chromium.org/236001/diff/5001/5003#newcode158
Line 158: 
On 2009/09/26 01:05:37, James Su wrote:
> In real life, 'xx' or 'yy' may exist. But it's ok to use them in this test, as
> only the locales listed in above filenames[] array are valid for this test.

As of today, 'xx' and 'yy' are not assigned to any language (in the future, they
may) :-). Anyway, it doesn't really matter.

> 
> On 2009/09/25 18:36:05, Jungshik Shin wrote:
> > In addition to the above, the actual case mentioned in the bug (Breton at
the
> > beginning of the list) may be useful. It's less robust than using 'xx', 'yy'
> > because one day we may support that language (although not so likely). On
the
> > other hand, it's a real-life example.  
> 
>

http://codereview.chromium.org/236001/diff/11001/11002
File app/l10n_util.cc (right):

http://codereview.chromium.org/236001/diff/11001/11002#newcode461
Line 461: // Only fallback to the system locale if LANGUAGE is not specified.
Can you add a comment that we're emulating gettext's behavior of ignoring
LC_{MESSAGES,ALL}/LANG when LANGUAGE is specified? 
The same comment in the unittest would help, too.