ChromeOS: should not show "Language changed" notification for certain languages.

ChromeOS: should not show "Language changed" notification for certain languages.

BUG=317718
TEST=manually tested

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

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

[Alexander Alekseev, 2014-07-10 21:21:11]

Please review:

jshin@: ui/base/l10n/*
nkostylev@: all

[Nikita (slow), 2014-07-14 09:37:42]

https://codereview.chromium.org/382973002/diff/1/chrome/browser/chromeos/loca...
File chrome/browser/chromeos/locale_change_guard.cc (right):

https://codereview.chromium.org/382973002/diff/1/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:40: bool
ShouldShowLanguageNotification(const std::string& from_locale,
This function might as well have unit_test.

https://codereview.chromium.org/382973002/diff/1/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:40: bool
ShouldShowLanguageNotification(const std::string& from_locale,
nit: Language -> Locale

https://codereview.chromium.org/382973002/diff/1/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:48: const char* const
ignore_languages[] = {"en", "de", "fr", "it"};
Do you think that pt* (pt/pt-BR/pt-PT) should not be on this list?

https://codereview.chromium.org/382973002/diff/1/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:49: if
(std::find(ignore_languages,
return !std::find(...);

https://codereview.chromium.org/382973002/diff/1/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:180: // Locale change detected
nit: dot at the end.

[Alexander Alekseev, 2014-07-14 22:01:59]

https://codereview.chromium.org/382973002/diff/1/chrome/browser/chromeos/loca...
File chrome/browser/chromeos/locale_change_guard.cc (right):

https://codereview.chromium.org/382973002/diff/1/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:40: bool
ShouldShowLanguageNotification(const std::string& from_locale,
On 2014/07/14 09:37:42, Nikita Kostylev wrote:
> nit: Language -> Locale

Done.

https://codereview.chromium.org/382973002/diff/1/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:40: bool
ShouldShowLanguageNotification(const std::string& from_locale,
On 2014/07/14 09:37:42, Nikita Kostylev wrote:
> This function might as well have unit_test.

Done.

https://codereview.chromium.org/382973002/diff/1/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:48: const char* const
ignore_languages[] = {"en", "de", "fr", "it"};
On 2014/07/14 09:37:41, Nikita Kostylev wrote:
> Do you think that pt* (pt/pt-BR/pt-PT) should not be on this list?

Actually no. We should show notification for "pt".

https://codereview.chromium.org/382973002/diff/1/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:49: if
(std::find(ignore_languages,
On 2014/07/14 09:37:41, Nikita Kostylev wrote:
> return !std::find(...);

Done.

https://codereview.chromium.org/382973002/diff/1/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:180: // Locale change detected
On 2014/07/14 09:37:41, Nikita Kostylev wrote:
> nit: dot at the end.

Done.

[Nikita (slow), 2014-07-15 13:51:56]

lgtm

[jungshik at Google, 2014-07-16 23:29:05]

I added a comment/question to the bug. I'm not sure about the decision on en-US
<-> en-GB.

https://codereview.chromium.org/382973002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/locale_change_guard_unittest.cc (right):

https://codereview.chromium.org/382973002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/locale_change_guard_unittest.cc:173:
LocaleChangeGuard::ShouldShowLocaleChangeNotification("en-AU", "en-GB"));
Please, add test for pairs of 'es-419' and 'es-ES' (or 'es'). 

How about 'en-US' <-> 'en-GB'?  You don't want to show the notification, either,
for that case?

[Nikita (slow), 2014-07-22 15:58:50]

On 2014/07/16 23:29:05, Jungshik Shin wrote:
> I added a comment/question to the bug. I'm not sure about the decision on
en-US
> <-> en-GB.
> 
>
https://codereview.chromium.org/382973002/diff/60001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/locale_change_guard_unittest.cc (right):
> 
>
https://codereview.chromium.org/382973002/diff/60001/chrome/browser/chromeos/...
> chrome/browser/chromeos/locale_change_guard_unittest.cc:173:
> LocaleChangeGuard::ShouldShowLocaleChangeNotification("en-AU", "en-GB"));
> Please, add test for pairs of 'es-419' and 'es-ES' (or 'es'). 
> 
> How about 'en-US' <-> 'en-GB'?  You don't want to show the notification,
either,
> for that case?

I think we don't need notification in that case either.
See https://code.google.com/p/chromium/issues/detail?id=317718#c23 and that bug
report originally started this issue.

please take a look

[Nikita (slow), 2014-08-12 08:32:49]

Jungshik, ping.

[jungshik at Google, 2014-08-14 00:13:14]

On 2014/08/12 08:32:49, Nikita Kostylev wrote:
> Jungshik, ping.

Really sorry for the terrible delay. I'm not yet convinced that {en-US, en-GB}
pair should be treated differently than {pt-BR, pt-PT} and {es-ES, es-419}
pairs. It's a bit heavy-handed, but potentially, one can pass 'fromLocale' and
'toLocale' to 'CheckAndResolveLocale()' in l10nutil and compare the return
value. With that, en-US and en-GB would be regarded as different, but it'd be
consistent with other pairs mentioned above. 

If there's a very compelling reason to treat {en-US, en-GB} pair differently
than {pt-BR, pt-PT} and {es-ES, es-419} pairs, I'd go with this CL, but I don't
see that at the moment. 

Alternatively, one can treat both Portugese and Spanish pairs in the same manner
as two English variants (i.e. not trigger a locale-switch message), but I'm
reluctant to do that..

[Nikita (slow), 2014-08-14 16:06:40]

On 2014/08/14 00:13:14, Jungshik Shin wrote:
> On 2014/08/12 08:32:49, Nikita Kostylev wrote:
> > Jungshik, ping.
> 
> If there's a very compelling reason to treat {en-US, en-GB} pair differently
> than {pt-BR, pt-PT} and {es-ES, es-419} pairs, I'd go with this CL, but I
don't
> see that at the moment. 

We've already discussed this in the email thread, I can bump that up and summary
is here:
https://code.google.com/p/chromium/issues/detail?id=317718#c12

If you have strong arguments on showing such notification, please reply in email
thread since it's where this decision has been made.

I really don't see clear user benefit in showing notification like "your locale
has changed from en-US to en-GB".

[jungshik at Google, 2014-08-15 12:25:01]

On 2014/08/14 16:06:40, Nikita Kostylev wrote:
> On 2014/08/14 00:13:14, Jungshik Shin wrote:
> > On 2014/08/12 08:32:49, Nikita Kostylev wrote:
> > > Jungshik, ping.
> > 
> > If there's a very compelling reason to treat {en-US, en-GB} pair differently
> > than {pt-BR, pt-PT} and {es-ES, es-419} pairs, I'd go with this CL, but I
> don't
> > see that at the moment. 
> 
> We've already discussed this in the email thread, I can bump that up and
summary
> is here:
> https://code.google.com/p/chromium/issues/detail?id=317718#c12
> 
> If you have strong arguments on showing such notification, please reply in
email
> thread since it's where this decision has been made.
 
> I really don't see clear user benefit in showing notification like "your
locale
> has changed from en-US to en-GB".

So, the difference between en-US and en-GB (even though we have sep is assumed
to be much smaller than the difference between other pairs (pt* and es*)?  Well,
I like to see more consistency across language pairs, but this is not a big
deal. 

For now, LGTM.  We may revisit it later.

[Alexander Alekseev, 2014-08-15 16:07:43]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-08-15 16:09:51]

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

[commit-bot: I haz the power, 2014-08-15 16:23:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/42332)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/47535)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-15 16:26:38]

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

[commit-bot: I haz the power, 2014-08-15 16:26:40]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/42339)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Alexander Alekseev, 2014-08-15 16:44:16]

https://codereview.chromium.org/382973002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/locale_change_guard_unittest.cc (right):

https://codereview.chromium.org/382973002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/locale_change_guard_unittest.cc:173:
LocaleChangeGuard::ShouldShowLocaleChangeNotification("en-AU", "en-GB"));
On 2014/07/16 23:29:04, Jungshik Shin wrote:
> Please, add test for pairs of 'es-419' and 'es-ES' (or 'es'). 
> 
> How about 'en-US' <-> 'en-GB'?  You don't want to show the notification,
either,
> for that case?  

Done.

[Alexander Alekseev, 2014-08-15 16:44:21]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-08-15 16:47:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alemate@chromium.org/382973002/90001

[commit-bot: I haz the power, 2014-08-15 19:49:39]

Committed patchset #6 (90001) as 289985

[miu, 2014-08-15 21:02:23]

A revert of this CL (patchset #6) has been created in
https://codereview.chromium.org/471403005/ by miu@chromium.org.

The reason for reverting is: Link failure of browser_tests on ChromiumOS build
bot:

http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=Linux%20C...
.

[Alexander Alekseev, 2014-08-18 12:55:34]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-08-18 12:55:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alemate@chromium.org/382973002/110001

[commit-bot: I haz the power, 2014-08-18 14:33:43]

Committed patchset #7 (110001) as 290255