Do not switch the ICU locale when interacting with public session pods

Do not switch the ICU locale when interacting with public session pods

When the user is choosing a language for a public session, Chrome needs
to verify that the selected locale is actually available, which can be
done by calling GetApplicationLocale(). However, that method has side
effects, in that it not only resolves the locale but also sets the ICU
default locale. This causes the ICU default locale to change
inadvertently while the user is interacting with the language picker.
This CL introduces a variant of GetApplicationLocale() without side
effects and uses that for public session pods.

BUG=403553
TEST=Extended unit and browser tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291033

[bartfab (slow), 2014-08-19 13:01:00]

Hi Tony,

Could you review ui/base/l10n/l10n_util* please?

Hi Pavel,

Could you review  chrome/browser/ui/webui/chromeos/login/l10n_util.cc please?

Hi Philipp,

Could you review
chrome/browser/chromeos/policy/device_local_account_browsertest.cc please?

[pneubeck (no reviews), 2014-08-19 15:09:30]

https://codereview.chromium.org/486203002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://codereview.chromium.org/486203002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1422:
EXPECT_EQ(l10n_util::GetLanguage(initial_locale_),
from reading this file and the function declaration of
BrowserProcess::GetApplicationLocale(), it's unclear why you would even mention
icu::Locale::getDefault() in this file.

Could you at least wrap the comparison like here (compare only initial_locale_
to Locale::getDefault)
and like in lines 1437-1439 into helper functions, where you document the reason
for the comparison to icu::Locale::getDefault() ?

https://codereview.chromium.org/486203002/diff/20001/ui/base/l10n/l10n_util.h
File ui/base/l10n/l10n_util.h (right):

https://codereview.chromium.org/486203002/diff/20001/ui/base/l10n/l10n_util.h...
ui/base/l10n/l10n_util.h:50: UI_BASE_EXPORT std::string
GetApplicationLocale(const std::string& pref_locale);
just for consideration:
shouldn't this be renamed to something that indicates the modification?
since this change will be merged, probably in a follow up.

[bartfab (slow), 2014-08-19 17:03:02]

https://codereview.chromium.org/486203002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://codereview.chromium.org/486203002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1422:
EXPECT_EQ(l10n_util::GetLanguage(initial_locale_),
On 2014/08/19 15:09:30, pneubeck wrote:
> from reading this file and the function declaration of
> BrowserProcess::GetApplicationLocale(), it's unclear why you would even
mention
> icu::Locale::getDefault() in this file.
> 
> Could you at least wrap the comparison like here (compare only initial_locale_
> to Locale::getDefault)
> and like in lines 1437-1439 into helper functions, where you document the
reason
> for the comparison to icu::Locale::getDefault() ?

I added comments and made this code a bit more concise. I specifically did not
want to move the comparisons to helper functions. This is because a failure in
the main test body gives you a useful line number while a failure in a helper
function only points you at the helper, making it impossible to tell which
invocation of the helper failed.

https://codereview.chromium.org/486203002/diff/20001/ui/base/l10n/l10n_util.h
File ui/base/l10n/l10n_util.h (right):

https://codereview.chromium.org/486203002/diff/20001/ui/base/l10n/l10n_util.h...
ui/base/l10n/l10n_util.h:50: UI_BASE_EXPORT std::string
GetApplicationLocale(const std::string& pref_locale);
On 2014/08/19 15:09:30, pneubeck wrote:
> just for consideration:
> shouldn't this be renamed to something that indicates the modification?
> since this change will be merged, probably in a follow up.

I agree. It came as a very nasty surprise to me that a method named Get*() has a
side effect that actually sets the locale. However, I would definitely not want
to do any such renames in a CL that is meant for merging.

[tony, 2014-08-19 18:44:50]

https://codereview.chromium.org/486203002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/l10n_util.cc (right):

https://codereview.chromium.org/486203002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/l10n_util.cc:463:
&l10n_util::GetApplicationLocale;
Do you have access to g_browser_process or does that not exist for the login
screen?  If you have access to g_browser_process, you should use 
g_browser_process->GetApplicationLocale() instead of
l10n_util::GetApplicationLocale.

[bartfab (slow), 2014-08-20 08:46:25]

https://codereview.chromium.org/486203002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/l10n_util.cc (right):

https://codereview.chromium.org/486203002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/l10n_util.cc:463:
&l10n_util::GetApplicationLocale;
On 2014/08/19 18:44:49, tony wrote:
> Do you have access to g_browser_process or does that not exist for the login
> screen?  If you have access to g_browser_process, you should use 
> g_browser_process->GetApplicationLocale() instead of
> l10n_util::GetApplicationLocale.

I do have access to g_browser_process but the semantics of
g_browser_process->GetApplicationLocale() (1) and those of
l10n_util::GetApplicationLocale() (1) are different:

(1) will return the current locale. This is of no interest to me.
(2) will check whether the locale passed in as a string is valid and if so,
return it. If the locale is not valid, it will fall back to en-US.

I need the semantics of (2), not (1).

[pneubeck (no reviews), 2014-08-20 09:03:40]

lgtm

https://codereview.chromium.org/486203002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://codereview.chromium.org/486203002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1422:
EXPECT_EQ(l10n_util::GetLanguage(initial_locale_),
On 2014/08/19 17:03:01, bartfab wrote:
> On 2014/08/19 15:09:30, pneubeck wrote:
> > from reading this file and the function declaration of
> > BrowserProcess::GetApplicationLocale(), it's unclear why you would even
> mention
> > icu::Locale::getDefault() in this file.
> > 
> > Could you at least wrap the comparison like here (compare only
initial_locale_
> > to Locale::getDefault)
> > and like in lines 1437-1439 into helper functions, where you document the
> reason
> > for the comparison to icu::Locale::getDefault() ?
> 
> I added comments and made this code a bit more concise. I specifically did not
> want to move the comparisons to helper functions. This is because a failure in
> the main test body gives you a useful line number while a failure in a helper
> function only points you at the helper, making it impossible to tell which
> invocation of the helper failed.

FROM_HERE ? :-)
I suggested that to keep the redundancy low, now you have a 10x duplication of a
two-line comment. And if you have to ever change this...

Anyways it's not dramatic, so your choice.

[dzhioev (left Google), 2014-08-20 11:52:05]

On 2014/08/19 13:01:00, bartfab wrote:
> Hi Tony,
> 
> Could you review ui/base/l10n/l10n_util* please?
> 
> Hi Pavel,
> 
> Could you review  chrome/browser/ui/webui/chromeos/login/l10n_util.cc please?
> 
> Hi Philipp,
> 
> Could you review
> chrome/browser/chromeos/policy/device_local_account_browsertest.cc please?

LGTM for login/l10n_util.cc

[tony, 2014-08-20 16:45:21]

LGTM, please have a bug for renaming the function after the merge to make it
more descriptive.

[bartfab (slow), 2014-08-20 17:52:28]

https://codereview.chromium.org/486203002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://codereview.chromium.org/486203002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1422:
EXPECT_EQ(l10n_util::GetLanguage(initial_locale_),
On 2014/08/20 09:03:40, pneubeck wrote:
> On 2014/08/19 17:03:01, bartfab wrote:
> > On 2014/08/19 15:09:30, pneubeck wrote:
> > > from reading this file and the function declaration of
> > > BrowserProcess::GetApplicationLocale(), it's unclear why you would even
> > mention
> > > icu::Locale::getDefault() in this file.
> > > 
> > > Could you at least wrap the comparison like here (compare only
> initial_locale_
> > > to Locale::getDefault)
> > > and like in lines 1437-1439 into helper functions, where you document the
> > reason
> > > for the comparison to icu::Locale::getDefault() ?
> > 
> > I added comments and made this code a bit more concise. I specifically did
not
> > want to move the comparisons to helper functions. This is because a failure
in
> > the main test body gives you a useful line number while a failure in a
helper
> > function only points you at the helper, making it impossible to tell which
> > invocation of the helper failed.
> 
> FROM_HERE ? :-)
> I suggested that to keep the redundancy low, now you have a 10x duplication of
a
> two-line comment. And if you have to ever change this...
> 
> Anyways it's not dramatic, so your choice.

OK, you got me convinced. I moved the locale checks into helper methods that
existed already anyway and one additional helper that I could have extracted a
while ago. Things are cleaner and shorter that way.

[bartfab (slow), 2014-08-20 17:52:53]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-08-20 17:53:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bartfab@chromium.org/486203002/60001

[bartfab (slow), 2014-08-20 17:56:28]

On 2014/08/20 16:45:21, tony wrote:
> LGTM, please have a bug for renaming the function after the merge to make it
> more descriptive.

I filed crbug.com/405610 for this.

[commit-bot: I haz the power, 2014-08-20 20:44:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-20 23:04:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-20 23:04:10]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bartfab (slow), 2014-08-21 09:11:06]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-08-21 09:12:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bartfab@chromium.org/486203002/60001

[commit-bot: I haz the power, 2014-08-21 09:49:15]

Committed patchset #4 (60001) as 291033