Default to current UI locale when recommended locales are invalid

Default to current UI locale when recommended locales are invalid

If the recommended locales for a public session are invalid/unavailable,
public sessions should default to the current UI language, not the
locale that happens to be first in alphabetic order (Arabic for Chrome OS
running in English).

BUG=403550
TEST=Extended unit and browser tests

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

[bartfab (slow), 2014-08-19 16:34:30]

Hi Pavel,

Could you review please?

Hi Philipp,

Could you take a look at device_local_account_browsertest.cc please?

[dzhioev (left Google), 2014-08-20 17:00:24]

LGTM with nits

https://codereview.chromium.org/483523005/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(left):

https://codereview.chromium.org/483523005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:140: }
Nothing will change if you drop this if-block completely.

https://codereview.chromium.org/483523005/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/483523005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:167: const
std::vector<std::string>* recommended_locales) {
This method is never called with |recommended_locales == NULL| so you can use
const ref instead of const ptr.

[bartfab (slow), 2014-08-20 18:40:06]

https://codereview.chromium.org/483523005/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(left):

https://codereview.chromium.org/483523005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:140: }
On 2014/08/20 17:00:24, dzhioev wrote:
> Nothing will change if you drop this if-block completely.

The update sent to the UI is the same as in the next block. But the update of
|public_session_recommended_locales_| is different: The first block removes an
entry, the second adds/updates an entry.

https://codereview.chromium.org/483523005/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/483523005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:167: const
std::vector<std::string>* recommended_locales) {
On 2014/08/20 17:00:24, dzhioev wrote:
> This method is never called with |recommended_locales == NULL| so you can use
> const ref instead of const ptr.

Done.

[dzhioev (left Google), 2014-08-20 19:26:36]

https://codereview.chromium.org/483523005/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(left):

https://codereview.chromium.org/483523005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:140: }
On 2014/08/20 18:40:05, bartfab wrote:
> On 2014/08/20 17:00:24, dzhioev wrote:
> > Nothing will change if you drop this if-block completely.
> 
> The update sent to the UI is the same as in the next block. But the update of
> |public_session_recommended_locales_| is different: The first block removes an
> entry, the second adds/updates an entry.
Yes, but nothing will change, if you allow empty entries in
|public_session_recommended_locales_| instead of removing them.
I claim that removing first if-block will not change behavior of code.

[pneubeck (no reviews), 2014-08-21 07:29:15]

lgtm with a question

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

https://codereview.chromium.org/483523005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1763: //
Click on the pod to expand it. Verify that the pod expands to its basic
Are invalid locales filtered before showing them in the UI?
Shouldn't they be filtered before even applying the policy, and thus this test
being at the policy handler?

[bartfab (slow), 2014-08-21 09:03:33]

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

https://codereview.chromium.org/483523005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1763: //
Click on the pod to expand it. Verify that the pod expands to its basic
On 2014/08/21 07:29:15, pneubeck wrote:
> Are invalid locales filtered before showing them in the UI?
> Shouldn't they be filtered before even applying the policy, and thus this test
> being at the policy handler?

Locales cannot be filtered in the policy handler because you need a round trip
to the worker pool and back to verify whether a particular locale is valid
(Chrome checks whether a localization file for that language actually exists on
disk or not). Thus, chrome:policy will show "OK" for any list of strings. Only
when we get to the point of constructing the UI do we find out which of these
strings are actually valid locales.

[bartfab (slow), 2014-08-21 12:14:42]

https://codereview.chromium.org/483523005/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(left):

https://codereview.chromium.org/483523005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:140: }
On 2014/08/20 19:26:35, dzhioev wrote:
> On 2014/08/20 18:40:05, bartfab wrote:
> > On 2014/08/20 17:00:24, dzhioev wrote:
> > > Nothing will change if you drop this if-block completely.
> > 
> > The update sent to the UI is the same as in the next block. But the update
of
> > |public_session_recommended_locales_| is different: The first block removes
an
> > entry, the second adds/updates an entry.
> Yes, but nothing will change, if you allow empty entries in
> |public_session_recommended_locales_| instead of removing them.
> I claim that removing first if-block will not change behavior of code.

I simplified this block a lot now. It is a tad less efficient but that makes no
difference in practice.

[bartfab (slow), 2014-08-21 12:15:19]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-08-21 12:16:23]

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

[commit-bot: I haz the power, 2014-08-21 18:06:30]

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/...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bartfab (slow), 2014-08-21 18:11:26]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-08-21 18:14:01]

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

[commit-bot: I haz the power, 2014-08-21 21:29:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-21 22:19:40]

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

[commit-bot: I haz the power, 2014-08-21 22:19:41]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bartfab (slow), 2014-08-22 12:49:14]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-08-22 12:50:16]

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

[commit-bot: I haz the power, 2014-08-22 13:42:54]

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

[commit-bot: I haz the power, 2014-08-22 13:46:57]

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

[commit-bot: I haz the power, 2014-08-22 13:46:58]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[bartfab (slow), 2014-08-22 15:12:48]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-08-22 15:13:41]

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

[commit-bot: I haz the power, 2014-08-22 15:25:21]

Committed patchset #6 (100001) as 291408