Allow recommended locales to be set for public sessions

Allow recommended locales to be set for public sessions

This CL introduces the |SessionLocales| policy that can be used to
recommend one or more UI locales for a public session. If the policy is
set, the first recommended locale is pre-selected when starting a public
session. If more than one locale is recommended, the public session pod
is shown in its advanced form, highlighting the availability of language
and keyboard layout pickers.

The keyboard layout is automatically set to the most popular layout
matching the UI locale.

This CL also fixes the flaky DeviceLocalAccountTest.* and
TermsOfServiceDownloadTest.Ü.

BUG=214904, 241790, 401010
TEST=Extensive browser test coverage, including picker UI

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

[bartfab (slow), 2014-07-30 00:41:57]

This is the final CL that wires up the public session locale policy.

Hi Nikita,

I would have sent it to Pavel but I know he is sick and I need this for M38.
Could you review:

chrome/browser/chromeos/login/*
chrome/browser/ui/webui/chromeos/*
ui/login/account_picker/*

Hi Philipp,

Could you review the policy parts of this CL:

chrome/browser/chromeos/policy/device_local_account_browsertest.cc
chrome/browser/policy/configuration_policy_handler_list_factory.cc
components/policy/resources/policy_templates.json

Hi Alexei,

Could you review:

tools/metrics/histograms/histograms.xml

[Nikita (slow), 2014-07-30 11:22:59]

lgtm with nits

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/app_launch_signin_screen.cc (right):

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/app_launch_signin_screen.cc:203: *it, true, false,
initial_auth_type, NULL, user_dict);
nit: /* public_session_recommended_locales */

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

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:121:
policy::BrowserPolicyConnectorChromeOS* policy_connector =
I suggest extracting code that's under if (is_public_account) condition into a
separate helper function.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:133: // Set
|kKeyInitialLocales| to the list of available locales. This list
nit: insert empty line before comment here and below.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:353:
public_session_recommended_locales_.find(user_id) ==
nit: please extract public_session_recommended_locales parameter to a local tmp
variable.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:450:
new_recommended_locales.clear();
NOTREACHED()?

https://codereview.chromium.org/426063005/diff/1/ui/login/account_picker/user...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/426063005/diff/1/ui/login/account_picker/user...
ui/login/account_picker/user_pod_row.js:1094: keyboardSelect.loadedLocale ==
locale ? keyboardSelect.value : "";
nit: ''

[pneubeck (no reviews), 2014-07-30 15:23:19]

lgtm

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:620:
ASSERT_TRUE(wizard_controller);
just FYI: ASSERTs in functions only skip the rest of the function. They don't
abort the whole test.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/426063005/diff/1/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:787:
handlers->AddHandler(make_scoped_ptr<ConfigurationPolicyHandler>(
Does the empty list need some special handling at an early stage?
Like ignoring the policy all together?

https://codereview.chromium.org/426063005/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/426063005/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6553: If this policy is not
set, the current UI locale will be pre-selected.
Maybe more clearly partition this description into the following parts:

- Short summary (current line 6547)
- Behavior independent of policy (lines 6549 without last sentence, 6555)
- Behavior without the policy (line 6553)
- Behavior with the policy (last sentence of 6549, lines 6557-6561)
- Additional notes (line 6563)

https://codereview.chromium.org/426063005/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6557: If there is more than
one recommended locale, it is assumed that users will want to select among these
locales. Locale and keyboard layout selection is prominently offered when
starting a public session.
In which order are the recommended locales shown? Alphabetically or as set in
the policy?

https://codereview.chromium.org/426063005/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6559: If there is no or only
one recommended locale, it is assumed that most users will want to use the
pre-selected locale. Locale and keyboard layout selection is less prominently
offered when starting a public session.
'no or' implies that setting an empty list of recommended locales is supported.
In line 6547 it says 'one or more'.

Maybe mention the empty list only as an 'error' which will be ignored?

https://codereview.chromium.org/426063005/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6561: When this policy is set
and automatic login is enabled (see the |DeviceLocalAccountAutoLoginId| and
|DeviceLocalAccountAutoLoginDelay| policies), the automatically started public
session will use the first recommended locale and the most popular keyboard
layout matching this locale.
The second half of the last sentence is redundant to line 6555.
Maybe drop?

https://codereview.chromium.org/426063005/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6563: This policy can only be
set as recommended. You can use this policy to move a set of recommended locales
to the top but users are always allowed to choose any locale supported by <ph
name="PRODUCT_OS_NAME">$2<ex>Google Chrome OS</ex></ph> for their session.
Hm. So we don't have a 'features' flag to declare 'can_be_mandatory':false ?

[pneubeck (no reviews), 2014-07-30 15:39:24]

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1344:
IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, NoRecommendedLocale) {
is this case different from no locale policy being set at all?

i didn't check the whole file, but is there a test that switches the locale if
the policy is not set?

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1473: //
Verify that the first recommended locale is selected.
It's irritating that you're testing this before the pod is expanded.
Better move it to after expanding it.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1475:
base::StringPrintf(
This block and the next together should test:
 - first locale in the list is selected (no matter what the content of the list)
 - the prefix of the list is matching the policy (in a certain order)
 - the rest of the list is containing other locales (and maybe that it doesn't
repeat the recommended locales)

not sure whether only the comments don't mention this or whether you're indeed
not testing it.

Your call, what of the above is worth testing (depends on how you implemented
it).

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1487: // form
directly as there are two or more recommended locales.
nit: "as there are more than two"

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1505: }
nit: empty line after this one.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1604: 
will continue review after this part later.

[pneubeck (no reviews), 2014-07-30 17:55:44]

IMO, the browser test is already very thorough. lgtm (no for real ;-)

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:2: // Use of
this source code is governed by a BSD-style license that can be
How about breaking this into several files according to categories of what's
tested and a base test implementation? (if so, should be in a follow up)

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:631:
input_method_manager->GetInputMethodUtil()->
I don't know how the logic for selecting the keyboard layout looks like after
your latest changes.
But is this function applying the exact same logic/algorithm?

You might consider calling to the same production function, which should have
it's own unit tests,
or to replace this logic by concrete test values.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1391:
session_locales_proto->mutable_value()->add_entries(kRecommendedLocales1[0]);
instead of using the unrelated kRecommendedLocales1 array, better define a
separate constant local to this test and reuse it in line 1426

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1430:
IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, MultipleRecommendedLocales) {
can the user also minify the pod after expanding? what should happen to the
locale selection?
just asking, might not make sense to test.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1432:
em::StringListPolicyProto* session_locales_proto =
the setting of the locales could be in helper functions.
taking an array+size. where you set a single locale or empty list, you could
define an according array constant.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1524:
value_ptr.reset(base::JSONReader::Read(json));
if this code part (JSONReader -> cast to concrete value type) is used more often
than the two times that you added, it could be moved to a helper (very likely,
beyond this browser test, this is done a hundred times...)

regarding error logging, the different error cases could be LOG(ERROR)'ed and
the result ASSERTed here.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1566: //
Verify that the first manually selected locale is still selected.
what means 'the first manually selected' ?
Can the user select more than one? Maybe change to 'Verify that the manually
selected locale...'

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1572: // The
UI will have requested an updated list of keyboard layouts at this
nit: move to a helper "FlushFileThread"

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1598: //
Verify that the locale and keyboard layout have been applied.
'Verify that the manually selected ...'

Hm. Is this locale / keyboard layout independent of the settings _on the login
screen_ ?
Is there yet-another menu to change the layout on the login screen?
Shouldn't they be coupled? If not, this could be rather confusing to the user.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1606:
AutoLoginWithoutRecommendedLocales) {
can you compare this to whatever other test maybe already exists for the auto
login?

[bartfab (slow), 2014-07-31 08:54:17]

The CQ bit was checked by bartfab@chromium.org

[bartfab (slow), 2014-07-31 08:56:11]

The CQ bit was unchecked by bartfab@chromium.org

[bartfab (slow), 2014-08-05 11:26:32]

Hi Alexei,

Ping about histograms.xml.

[Alexei Svitkine (slow), 2014-08-05 14:24:23]

lgtm, sorry for the delay

[bartfab (slow), 2014-08-05 17:16:25]

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/app_launch_signin_screen.cc (right):

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/app_launch_signin_screen.cc:203: *it, true, false,
initial_auth_type, NULL, user_dict);
On 2014/07/30 11:22:59, Nikita Kostylev wrote:
> nit: /* public_session_recommended_locales */

Done.

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

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:121:
policy::BrowserPolicyConnectorChromeOS* policy_connector =
On 2014/07/30 11:22:59, Nikita Kostylev wrote:
> I suggest extracting code that's under if (is_public_account) condition into a
> separate helper function.

Done.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:133: // Set
|kKeyInitialLocales| to the list of available locales. This list
On 2014/07/30 11:22:59, Nikita Kostylev wrote:
> nit: insert empty line before comment here and below.

Done.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:353:
public_session_recommended_locales_.find(user_id) ==
On 2014/07/30 11:22:59, Nikita Kostylev wrote:
> nit: please extract public_session_recommended_locales parameter to a local
tmp
> variable.

Done.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:450:
new_recommended_locales.clear();
On 2014/07/30 11:22:59, Nikita Kostylev wrote:
> NOTREACHED()?

Done.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:2: // Use of
this source code is governed by a BSD-style license that can be
On 2014/07/30 17:55:44, pneubeck wrote:
> How about breaking this into several files according to categories of what's
> tested and a base test implementation? (if so, should be in a follow up)

Good idea. I filed crbug.com/400702 against myself to keep track of this.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:620:
ASSERT_TRUE(wizard_controller);
On 2014/07/30 15:23:19, pneubeck wrote:
> just FYI: ASSERTs in functions only skip the rest of the function. They don't
> abort the whole test.

That's why I use ASSERT_NO_FATAL_FAILURE().

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:631:
input_method_manager->GetInputMethodUtil()->
On 2014/07/30 17:55:44, pneubeck wrote:
> I don't know how the logic for selecting the keyboard layout looks like after
> your latest changes.
> But is this function applying the exact same logic/algorithm?
> 
> You might consider calling to the same production function, which should have
> it's own unit tests,
> or to replace this logic by concrete test values.

This is exactly the production function that the UI code calls. You feed it the
locale and you get back a list of input methods matching the locale, in priority
order. I could hard-code the result but there are differences between branded
and non-branded builds that I would have to hack around. I think it is better to
verify that the UI's behavior matches this function's output. And yes, as you
say, making sure that this function returns sensible values is the job of a
separate unit test (which actually does exist! :).

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1344:
IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, NoRecommendedLocale) {
On 2014/07/30 15:39:23, pneubeck wrote:
> is this case different from no locale policy being set at all?
> 
> i didn't check the whole file, but is there a test that switches the locale if
> the policy is not set?

There was no test verifying that scenario. I added one now,
NoRecommendedLocaleSwitch.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1391:
session_locales_proto->mutable_value()->add_entries(kRecommendedLocales1[0]);
On 2014/07/30 17:55:44, pneubeck wrote:
> instead of using the unrelated kRecommendedLocales1 array, better define a
> separate constant local to this test and reuse it in line 1426

Done.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1430:
IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, MultipleRecommendedLocales) {
On 2014/07/30 17:55:44, pneubeck wrote:
> can the user also minify the pod after expanding? what should happen to the
> locale selection?
> just asking, might not make sense to test.

Yes, the pod will contract if it loses focus. The pod should remember its state
if the user focuses it again. I extended the test to verify this.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1432:
em::StringListPolicyProto* session_locales_proto =
On 2014/07/30 17:55:44, pneubeck wrote:
> the setting of the locales could be in helper functions.
> taking an array+size. where you set a single locale or empty list, you could
> define an according array constant.

Done.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1473: //
Verify that the first recommended locale is selected.
On 2014/07/30 15:39:23, pneubeck wrote:
> It's irritating that you're testing this before the pod is expanded.
> Better move it to after expanding it.

Done.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1475:
base::StringPrintf(
On 2014/07/30 15:39:23, pneubeck wrote:
> This block and the next together should test:
>  - first locale in the list is selected (no matter what the content of the
list)

My test verifies this but the focus is slightly different: First, I verify that
the list starts with the recommended locales in the right order. Then, I verify
that the selected locale is the first recommended one. As a corollary of the
two, I can be sure that the first list entry is selected.

>  - the prefix of the list is matching the policy (in a certain order)

Yes, this is tested.

>  - the rest of the list is containing other locales (and maybe that it doesn't
> repeat the recommended locales)

Yes, this is tested - though I did not verify that none of the recommended
locales repeat in that list. I added that now.

> 
> not sure whether only the comments don't mention this or whether you're indeed
> not testing it.
> 
> Your call, what of the above is worth testing (depends on how you implemented
> it).

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1487: // form
directly as there are two or more recommended locales.
On 2014/07/30 15:39:23, pneubeck wrote:
> nit: "as there are more than two"

No, "two or more" is correct. The advanced pod is shown for 2, 3, 4, etc.
locales, not "more than two," which would be 3, 4, etc.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1505: }
On 2014/07/30 15:39:23, pneubeck wrote:
> nit: empty line after this one.

Done.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1524:
value_ptr.reset(base::JSONReader::Read(json));
On 2014/07/30 17:55:44, pneubeck wrote:
> if this code part (JSONReader -> cast to concrete value type) is used more
often
> than the two times that you added, it could be moved to a helper (very likely,
> beyond this browser test, this is done a hundred times...)
> 
> regarding error logging, the different error cases could be LOG(ERROR)'ed and
> the result ASSERTed here.

I do such a conversion three times in this file. I agree that if there was a
general helper, it would be nice to use it. But I am reluctant to start adding
one as that would be a large undertaking with rather little ROI.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1566: //
Verify that the first manually selected locale is still selected.
On 2014/07/30 17:55:44, pneubeck wrote:
> what means 'the first manually selected' ?
> Can the user select more than one? Maybe change to 'Verify that the manually
> selected locale...'

Oops, typo. Removed the "first".

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1572: // The
UI will have requested an updated list of keyboard layouts at this
On 2014/07/30 17:55:44, pneubeck wrote:
> nit: move to a helper "FlushFileThread"

Done.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1598: //
Verify that the locale and keyboard layout have been applied.
On 2014/07/30 17:55:44, pneubeck wrote:
> 'Verify that the manually selected ...'
> 
> Hm. Is this locale / keyboard layout independent of the settings _on the login
> screen_ ?
> Is there yet-another menu to change the layout on the login screen?
> Shouldn't they be coupled? If not, this could be rather confusing to the user.

There are several places where you can pick the locale and/or keyboard layout:

1) During OOBE, you can pick the initial locale and keyboard layout for the
device. You only do this once. It happens before you ever reach the login
screen. Changes apply immediately.
2) On the login screen, you can switch the currently active keyboard layout via
keyboard shortcuts or the ash tray bubble. Changes apply immediately.
3) During a session, you can switch the currently active keyboard layout via
keyboard shortcuts or the settings page. Changes apply immediately.
4) During a session, you can switch the locale via the settings page. Changes
apply after the next logout/login.

What I am adding is:
5) On the login screen, you can switch the keyboard layout and locale that will
be used for the public session you are about to start. Changes apply when the
session actually starts.

So technically, yes, you have access to 2) and 5) on the login screen. You could
use 2) to change the currently active keyboard layout to A, select layout B via
5) and then wonder whether you get A or B. This is unavoidable and is by design:

* Choices made for a public session (5) should never apply outside the session.
So we cannot switch to the selected locale and keyboard before the start of the
session.
* On the other hand, you should be able to switch the currently active keyboard
layout (2) because you may want to log in to another account that requires a
password and you want to use a particular layout to type in this password.

It follows that the correct behavior is:

* Honor layout A as long as you are on the login screen.
* Switch to layout B when you actually start the public session.

And this is exactly how things work.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1604: 
On 2014/07/30 15:39:23, pneubeck wrote:
> will continue review after this part later.

Acknowledged.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:1606:
AutoLoginWithoutRecommendedLocales) {
On 2014/07/30 17:55:44, pneubeck wrote:
> can you compare this to whatever other test maybe already exists for the auto
> login?

The only other auto-login tests I am aware of are in
ExistingUserControllerPublicSessionTest. The tests there verify that different
auto-login delays work as they should (zero delay, short delay, long delay). The
focus of the test here is different. It just uses a zero delay but verifies that
language and keyboard layout work as expected. I think both parts have their
merits.

https://codereview.chromium.org/426063005/diff/1/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/426063005/diff/1/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:787:
handlers->AddHandler(make_scoped_ptr<ConfigurationPolicyHandler>(
On 2014/07/30 15:23:19, pneubeck wrote:
> Does the empty list need some special handling at an early stage?
> Like ignoring the policy all together?

An empty list is permissible. It means the same thing as not setting the policy
at all of course: zero recommended locales. I cannot see why anyone would want
to set an empty list but it is technically valid and it is handled correctly by
the code.

https://codereview.chromium.org/426063005/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/426063005/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6553: If this policy is not
set, the current UI locale will be pre-selected.
On 2014/07/30 15:23:19, pneubeck wrote:
> Maybe more clearly partition this description into the following parts:
> 
> - Short summary (current line 6547)
> - Behavior independent of policy (lines 6549 without last sentence, 6555)
> - Behavior without the policy (line 6553)
> - Behavior with the policy (last sentence of 6549, lines 6557-6561)
> - Additional notes (line 6563)

Done.

https://codereview.chromium.org/426063005/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6557: If there is more than
one recommended locale, it is assumed that users will want to select among these
locales. Locale and keyboard layout selection is prominently offered when
starting a public session.
On 2014/07/30 15:23:19, pneubeck wrote:
> In which order are the recommended locales shown? Alphabetically or as set in
> the policy?

Clarified.

https://codereview.chromium.org/426063005/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6559: If there is no or only
one recommended locale, it is assumed that most users will want to use the
pre-selected locale. Locale and keyboard layout selection is less prominently
offered when starting a public session.
On 2014/07/30 15:23:19, pneubeck wrote:
> 'no or' implies that setting an empty list of recommended locales is
supported.
> In line 6547 it says 'one or more'.
> 
> Maybe mention the empty list only as an 'error' which will be ignored?

Recommending an empty list works but makes little sense as it is equivalent to
no recommendation at all. I simplified this sentence by using "Otherwise,". I
intentionally prefer not to go into a lengthy description of what happens when
there is an empty recommended list. Chrome will handle it 100% gracefully and
correctly but there is no point in setting an empty list, so I would rather not
complicate the policy description further with this corner case.

https://codereview.chromium.org/426063005/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6561: When this policy is set
and automatic login is enabled (see the |DeviceLocalAccountAutoLoginId| and
|DeviceLocalAccountAutoLoginDelay| policies), the automatically started public
session will use the first recommended locale and the most popular keyboard
layout matching this locale.
On 2014/07/30 15:23:19, pneubeck wrote:
> The second half of the last sentence is redundant to line 6555.
> Maybe drop?

No, it's not redundant. Line 6555 explains what keyboard layout will be
pre-selected for users going through the manual login flow. This sentence
describes what keyboard layout Chrome will pick automatically during auto-login.

https://codereview.chromium.org/426063005/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6563: This policy can only be
set as recommended. You can use this policy to move a set of recommended locales
to the top but users are always allowed to choose any locale supported by <ph
name="PRODUCT_OS_NAME">$2<ex>Google Chrome OS</ex></ph> for their session.
On 2014/07/30 15:23:19, pneubeck wrote:
> Hm. So we don't have a 'features' flag to declare 'can_be_mandatory':false ?

Correct, there is no such flag. It is not needed for now because at the moment,
the only two policies that do not support mandatory are for Chrome OS, where the
policy templates auto-generated from this file are not used. Once we get the
first policy that does not support mandatory on desktop, we should add a flag.

https://codereview.chromium.org/426063005/diff/1/ui/login/account_picker/user...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/426063005/diff/1/ui/login/account_picker/user...
ui/login/account_picker/user_pod_row.js:1094: keyboardSelect.loadedLocale ==
locale ? keyboardSelect.value : "";
On 2014/07/30 11:22:59, Nikita Kostylev wrote:
> nit: ''

Done.

[bartfab (slow), 2014-08-05 17:16:28]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-08-05 17:17:26]

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

[bartfab (slow), 2014-08-05 19:18:23]

The CQ bit was unchecked by bartfab@chromium.org

[bartfab (slow), 2014-08-05 19:18:47]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-08-05 19:19:29]

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

[bartfab (slow), 2014-08-05 19:58:31]

Committed patchset #3 manually as 287581 (presubmit successful).

[robliao, 2014-08-05 23:10:45]

On 2014/08/05 19:58:31, bartfab wrote:
> Committed patchset #3 manually as 287581 (presubmit successful).

Reverted by https://codereview.chromium.org/441143002/
due to buffer overflow in test.

[jam, 2014-08-06 02:08:37]

On 2014/08/05 23:10:45, robliao wrote:
> On 2014/08/05 19:58:31, bartfab wrote:
> > Committed patchset #3 manually as 287581 (presubmit successful).
> 
> Reverted by https://codereview.chromium.org/441143002/
> due to buffer overflow in test.

if there's a buffer overflow in a new test that this cl added, we should disable
that test only. as i understand it, this cl fixes the DeviceLocalAccountTest.*
and TermsOfServiceDownloadTestInstance.* failures on trybots which are causing a
large delay in CQ time as it recompiles and reruns browser_tests without the
patch.

[bartfab (slow), 2014-08-06 10:03:51]

On 2014/08/06 02:08:37, jam wrote:
> On 2014/08/05 23:10:45, robliao wrote:
> > On 2014/08/05 19:58:31, bartfab wrote:
> > > Committed patchset #3 manually as 287581 (presubmit successful).
> > 
> > Reverted by https://codereview.chromium.org/441143002/
> > due to buffer overflow in test.
> 
> if there's a buffer overflow in a new test that this cl added, we should
disable
> that test only. as i understand it, this cl fixes the DeviceLocalAccountTest.*
> and TermsOfServiceDownloadTestInstance.* failures on trybots which are causing
a
> large delay in CQ time as it recompiles and reruns browser_tests without the
> patch.

I agree with John. I will re-land the CL with the offending test disabled to
unblock the CQ ASAP and will then look into fixing the test.

[bartfab (slow), 2014-08-06 10:12:02]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-08-06 10:12:47]

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

[commit-bot: I haz the power, 2014-08-06 13:08:07]

Change committed as 287767