Add login screen locale and input method device policies

Add login screen locale and input method device policies

This CL introduces two device policies for Chrome OS:
+ DeviceLoginScreenLocales
  Mandates a specific locale for the login screen.
  (Note that the policy value is a list for future compatibility, in
  case dynamically switching login screen locales is implemented in the
  future. Currently only the first value is used).
+ DeviceLoginScreenInputMethods
  Enforces input methods on the login screen.

This is done by:
Locale:
- Introducing cros setting kDeviceLoginScreenLocales, controlled by the new policy
- Applying Login Screen locale before login screen is displayed
- Pre-setting kApplicationLocale on application exit for the next run

Input Methods:
- Introducing cros setting kDeviceLoginScreenInputMethods, controlled by the new policy
- new method SetAllowedKeyboardLayoutInputMethods on InputMethodManager to restrict input methods
- enforcing keyboard layouts (through InputMethodManager) on sign-in screen (depending on which user's fod is focused)
- adding info to the tray icon to display that input methods are controlled by policy

See go/cros-locale-input-policy-dd for more detail.

BUG=373324
TEST=New browser_tests, new unit_tests, manual test with YAPS.

Review-Url: https://codereview.chromium.org/2652793003
Cr-Commit-Position: refs/heads/master@{#448961}
Committed: https://chromium.googlesource.com/chromium/src/+/8c3ffb5e698796da543bcbf2192bbb5aebb90807

[pmarko, 2017-01-25 09:40:26]

Description was changed from

==========
Add login screen locale and input method device policies

This CL introduces two device policies for ChromeOS:
+ DeviceLoginScreenLocales
  Mandates a specific locale for the login screen.
  (Note that the policy value is a list for future compatibility, in
  case dynamically switching login screen locales is implemented in the
  future. Currently only the first value is used).
+ DeviceLoginScreenInputMethods
  Enforces input methods on the login screen.

BUG=373324
TEST=New browser_tests, new unit_tests, manual test with YAPS.
==========

to

==========
Add login screen locale and input method device policies

This CL introduces two device policies for Chrome OS:
+ DeviceLoginScreenLocales
  Mandates a specific locale for the login screen.
  (Note that the policy value is a list for future compatibility, in
  case dynamically switching login screen locales is implemented in the
  future. Currently only the first value is used).
+ DeviceLoginScreenInputMethods
  Enforces input methods on the login screen.

BUG=373324
TEST=New browser_tests, new unit_tests, manual test with YAPS.
==========

[pmarko, 2017-01-25 14:06:56]

Description was changed from

==========
Add login screen locale and input method device policies

This CL introduces two device policies for Chrome OS:
+ DeviceLoginScreenLocales
  Mandates a specific locale for the login screen.
  (Note that the policy value is a list for future compatibility, in
  case dynamically switching login screen locales is implemented in the
  future. Currently only the first value is used).
+ DeviceLoginScreenInputMethods
  Enforces input methods on the login screen.

BUG=373324
TEST=New browser_tests, new unit_tests, manual test with YAPS.
==========

to

==========
Add login screen locale and input method device policies

This CL introduces two device policies for Chrome OS:
+ DeviceLoginScreenLocales
  Mandates a specific locale for the login screen.
  (Note that the policy value is a list for future compatibility, in
  case dynamically switching login screen locales is implemented in the
  future. Currently only the first value is used).
+ DeviceLoginScreenInputMethods
  Enforces input methods on the login screen.

This is done by:
Locale:
- Introducing cros setting kDeviceLoginScreenLocales, controlled by the new
policy
- Applying Login Screen locale before login screen is displayed
- Pre-setting kApplicationLocale on application exit for the next run

Input Methods:
- Introducing cros setting kDeviceLoginScreenInputMethods, controlled by the new
policy
- new method SetAllowedKeyboardLayoutInputMethods on InputMethodManager to
restrict input methods
- enforcing keyboard layouts (through InputMethodManager) on sign-in screen
(depending on which user's fod is focused)
- adding info to the tray icon to display that input methods are controlled by
policy

See go/cros-locale-input-policy-dd for more detail.

BUG=373324
TEST=New browser_tests, new unit_tests, manual test with YAPS.
==========

[pmarko, 2017-01-25 14:12:17]

pmarko@chromium.org changed reviewers:
+ haraken@chromium.org, pastarmovj@chromium.org, pkasting@chromium.org,
shuchen@chromium.org, skuhne@chromium.org, thakis@chromium.org,
xiyuan@chromium.org

[pmarko, 2017-01-25 14:12:18]

xiyuan@chromium.org: Please review the following changes:
  ui/login/account_picker/user_pod_row.js
  chrome/browser/ui/webui/*
    Enforcing keyboard layouts depending on focused user pod on sign-in screen:

  chrome/browser/chromeos/login/*
    Enforcing locale before sing-in screen:
    Browser tests

  chromeos/settings/cros_settings_names.*
    The cros setting names for the feature:

haraken@chromium.org: Please review changes in 
 Histogram values for the new device policies
  tools/metrics/histograms/*

shuchen@chromium.org: Please review changes in 
  chrome/browser/chromeos/input_method/*
  ui/base/ime/chromeos/*
    Possiblity to restrict allowed keyboard layouts in InputMethodManager

skuhne@chromium.org: Please review changes in 
  ash/common/system/*
    Displaying a "Controlled Setting Indicator" in the keyboard tray when
keyboard layouts are restricted
    displaying the tray icon even for one IME when keyboard layouts are
restricted

pastarmovj@chromium.org: Please review changes in 
  chrome/browser/chromeos/policy/*
  chrome/browser/chromeos/settings/*
  components/policy/resources/policy_templates.json
    Policy parts and wiring to the cros settings.

pkasting@chromium.org: Please review changes in 
  chrome/browser/ui
    Wiring of InputMethodManager into SystemTrayDelegate

thakis@chromium.org: Please review changes in 
  chrome/browser/lifetime/*
    Setting of kApplicationLocale according to the cros setting for the next run

[pastarmovj, 2017-01-25 15:42:21]

mostly nits from my side.

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

https://codereview.chromium.org/2652793003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:857: if
(policy.has_login_screen_locales()) {
Hmm looking into the policies in this function it feels to me some belong in the
DecodeLoginPolicicies including yours. Can you go through the list and move some
there?

https://codereview.chromium.org/2652793003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/2652793003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_provider.cc:102:
kDeviceLoginScreenInputMethods};
Is the closing } put there by clang-format? If so keep it there if it is a
mistake bring it back below.

https://codereview.chromium.org/2652793003/diff/20001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2652793003/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:9468: 'desc': '''Configures
the locale which is enforced on the sign-in screen.''',
Please extend the documentation a little bit more. E.g. what happens when this
policy is not set. What happens if the locale is not correct etc. Also this
descriptions says "the locale" and it is a list so it can be more than one
locale, if only one is respected now but the plan is to support multiple
document this too.

https://codereview.chromium.org/2652793003/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:9480: 'dynamic_refresh': True,
Is it correct that one is dynamic refresh and the other isn't?

https://codereview.chromium.org/2652793003/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:9487: 'desc': '''Configures
which keyboard layouts are allowed on the sign-in screen.''',
Same as above.

[Mr4D (OOO till 08-26), 2017-01-25 17:31:54]

ash/common/system lgtm

[xiyuan, 2017-01-25 19:24:15]

https://codereview.chromium.org/2652793003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_screen_policy_browsertest.cc (right):

https://codereview.chromium.org/2652793003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_screen_policy_browsertest.cc:118:
ResourceBundle::CleanupSharedInstance();
Is this necessary? If so, could we comment why it is needed?

https://codereview.chromium.org/2652793003/diff/20001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/2652793003/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3348: chrome.send('noPodFocused');
We probably need to update user_manager_screen_handler.cc to bind a donothing
handler as "focusPod" [1] as well.

[1]:
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/signin/user_mana...

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

https://codereview.chromium.org/2652793003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:332:
EnforcePolicyInputMethods(std::string());
nit: call OnAllowedInputMethodsChanged intead

[Peter Kasting, 2017-01-25 22:09:30]

c/b/ui/ash/ LGTM

https://codereview.chromium.org/2652793003/diff/40001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/2652793003/diff/40001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:489: return
l10n_util::GetStringUTF16(IDS_OPTIONS_CONTROLLED_SETTING_POLICY);
Nit: Shorter, and avoids else after return:

  auto ime_state = input_method::InputMethodManager::Get()->GetActiveIMEState();
  return ime_state->GetAllowedKeyboardLayoutInputMethods().empty()
             ? base::string16()
             : l10n_util::GetStringUTF16(IDS_OPTIONS_CONTROLLED_SETTING_POLICY);

[pmarko, 2017-01-26 12:32:58]

Thank you for your comments, PTAL.

https://codereview.chromium.org/2652793003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_screen_policy_browsertest.cc (right):

https://codereview.chromium.org/2652793003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_screen_policy_browsertest.cc:118:
ResourceBundle::CleanupSharedInstance();
On 2017/01/25 19:24:14, xiyuan wrote:
> Is this necessary? If so, could we comment why it is needed?

You're right, we don't. Removed.

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

https://codereview.chromium.org/2652793003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:857: if
(policy.has_login_screen_locales()) {
On 2017/01/25 15:42:20, pastarmovj wrote:
> Hmm looking into the policies in this function it feels to me some belong in
the
> DecodeLoginPolicicies including yours. Can you go through the list and move
some
> there?

Done. I've moved:
existing:
  LoginScreenPowerManagement
  LoginScreenDomaingAutoComplete
new:
  LoginScreenLocales
  LoginScreenInputMethods

Also moved LoginScreenLocales+LoginScreeInputMethods from DecodeGenericPolicies
to DecodeLoginPolicies in device_settings_provider.cc

https://codereview.chromium.org/2652793003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/2652793003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_provider.cc:102:
kDeviceLoginScreenInputMethods};
On 2017/01/25 15:42:20, pastarmovj wrote:
> Is the closing } put there by clang-format? If so keep it there if it is a
> mistake bring it back below. 

clang format does that, but it looks better if I leave the trailing comma in
there (as was in the original version).
Done.

https://codereview.chromium.org/2652793003/diff/20001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2652793003/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:9468: 'desc': '''Configures
the locale which is enforced on the sign-in screen.''',
On 2017/01/25 15:42:20, pastarmovj wrote:
> Please extend the documentation a little bit more. E.g. what happens when this
> policy is not set. What happens if the locale is not correct etc. Also this
> descriptions says "the locale" and it is a list so it can be more than one
> locale, if only one is respected now but the plan is to support multiple
> document this too.

Done.

https://codereview.chromium.org/2652793003/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:9480: 'dynamic_refresh': True,
On 2017/01/25 15:42:20, pastarmovj wrote:
> Is it correct that one is dynamic refresh and the other isn't?

Yes, the reason is that changing locale dynamically is currently only supported
in very special situations (e.g. OOBE before login - the webui needs to support
reload, etc.).
The critical situation is when the login screen is displayed and the
DeviceLoginScreenLocales policy changes.
Currently, nothing happens until you restart the browser (hence dynamic_refresh:
False there).
I thought about re-starting the browser automatically, but after all I think
it's not really worth the added complexity. To do it properly, I would need to
add idle detection to not restart the browser when the user is typing something.

https://codereview.chromium.org/2652793003/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:9487: 'desc': '''Configures
which keyboard layouts are allowed on the sign-in screen.''',
On 2017/01/25 15:42:20, pastarmovj wrote:
> Same as above.

Done.

https://codereview.chromium.org/2652793003/diff/20001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/2652793003/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3348: chrome.send('noPodFocused');
On 2017/01/25 19:24:14, xiyuan wrote:
> We probably need to update user_manager_screen_handler.cc to bind a donothing
> handler as "focusPod" [1] as well.
> 
> [1]:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/signin/user_mana...

Done.
Good point!

https://codereview.chromium.org/2652793003/diff/40001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/2652793003/diff/40001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:489: return
l10n_util::GetStringUTF16(IDS_OPTIONS_CONTROLLED_SETTING_POLICY);
On 2017/01/25 22:09:30, Peter Kasting wrote:
> Nit: Shorter, and avoids else after return:
> 
>   auto ime_state =
input_method::InputMethodManager::Get()->GetActiveIMEState();
>   return ime_state->GetAllowedKeyboardLayoutInputMethods().empty()
>              ? base::string16()
>              :
l10n_util::GetStringUTF16(IDS_OPTIONS_CONTROLLED_SETTING_POLICY);

Thanks, done.

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

https://codereview.chromium.org/2652793003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:332:
EnforcePolicyInputMethods(std::string());
On 2017/01/25 19:24:14, xiyuan wrote:
> nit: call OnAllowedInputMethodsChanged intead

Done.

[xiyuan, 2017-01-26 16:09:44]

lgtm

[pastarmovj, 2017-01-26 20:13:16]

Some more nits but nothing to prevent me from issuing an lgtm concerning the
policy code.

https://codereview.chromium.org/2652793003/diff/60001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2652793003/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:9468: 'desc': '''Configures
the locale which is enforced on the Chrome OS sign-in screen.
I know this is very inconsistent right now but still I will appreciate if you
use the proper placeholders for the brand name <ph
name="PRODUCT_OS_NAME">$2<ex>Google Chrome OS</ex></ph> so that it get's
properly substituted by Chromium OS for the open source builds.

https://codereview.chromium.org/2652793003/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:9470: If this policy is set,
the Chrome OS sign-in screen will always be displayed in the locale which is
given by the first value of this policy (the policy is defined as a list for
forward compatibility).
Leave an empty line between paragraphs.

[pmarko, 2017-01-27 08:52:19]

Julian: PTAL

https://codereview.chromium.org/2652793003/diff/60001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2652793003/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:9468: 'desc': '''Configures
the locale which is enforced on the Chrome OS sign-in screen.
On 2017/01/26 20:13:16, pastarmovj wrote:
> I know this is very inconsistent right now but still I will appreciate if you
> use the proper placeholders for the brand name <ph
> name="PRODUCT_OS_NAME">$2<ex>Google Chrome OS</ex></ph> so that it get's
> properly substituted by Chromium OS for the open source builds.

Done.
I've actually left the placeholder only in the first paragraph and refer to
"sign-in screen" later to prevent redundancy.

https://codereview.chromium.org/2652793003/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:9470: If this policy is set,
the Chrome OS sign-in screen will always be displayed in the locale which is
given by the first value of this policy (the policy is defined as a list for
forward compatibility).
On 2017/01/26 20:13:16, pastarmovj wrote:
> Leave an empty line between paragraphs.

Done. (merged last 3 paragraphs into one)

[pmarko, 2017-01-27 08:57:13]

@xiyuan: BTW, just to double-check, have you taken a look at
chrome/browser/chromeos/login/ui/login_display_host_impl.cc ? I have forgotten
to mention it in my original message.
Thank you!

On 2017/01/27 08:52:19, pmarko wrote:
> Julian: PTAL
> 
>
https://codereview.chromium.org/2652793003/diff/60001/components/policy/resou...
> File components/policy/resources/policy_templates.json (right):
> 
>
https://codereview.chromium.org/2652793003/diff/60001/components/policy/resou...
> components/policy/resources/policy_templates.json:9468: 'desc': '''Configures
> the locale which is enforced on the Chrome OS sign-in screen.
> On 2017/01/26 20:13:16, pastarmovj wrote:
> > I know this is very inconsistent right now but still I will appreciate if
you
> > use the proper placeholders for the brand name <ph
> > name="PRODUCT_OS_NAME">$2<ex>Google Chrome OS</ex></ph> so that it get's
> > properly substituted by Chromium OS for the open source builds.
> 
> Done.
> I've actually left the placeholder only in the first paragraph and refer to
> "sign-in screen" later to prevent redundancy.
> 
>
https://codereview.chromium.org/2652793003/diff/60001/components/policy/resou...
> components/policy/resources/policy_templates.json:9470: If this policy is set,
> the Chrome OS sign-in screen will always be displayed in the locale which is
> given by the first value of this policy (the policy is defined as a list for
> forward compatibility).
> On 2017/01/26 20:13:16, pastarmovj wrote:
> > Leave an empty line between paragraphs.
> 
> Done. (merged last 3 paragraphs into one)

[xiyuan, 2017-01-27 15:38:05]

On 2017/01/27 08:57:13, pmarko wrote:
> @xiyuan: BTW, just to double-check, have you taken a look at
> chrome/browser/chromeos/login/ui/login_display_host_impl.cc ? I have forgotten
> to mention it in my original message.
> Thank you!
> 

Yes, looked through all except files under */policy/*.

[haraken, 2017-01-27 17:58:44]

tools/metrics/histograms/* LGTM

[pmarko, 2017-01-31 13:51:05]

pmarko@chromium.org changed reviewers:
+ yukishiino@chromium.org

[pmarko, 2017-01-31 13:52:11]

@xiyuan: Thanks, just wanted to double-check.

@Yuki: Would you mind reviewing ui/base/ime as Shu Chen is currently on
vacation?
Thank you!

[Yuki, 2017-01-31 15:18:30]

ui/base/ime/ LGTM with nits.

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/in...
File ui/base/ime/chromeos/input_method_manager.h (right):

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/in...
ui/base/ime/chromeos/input_method_manager.h:221: virtual bool
SetAllowedKeyboardLayoutInputMethods(
nit: "keyboard layout input method" sounds a bit weird to me because "input
method" in a concept containing direct input (with a variety of keyboard
layouts) and non-direct input (such as Chinese/Japanese/Korean).

I'd recommend to rename this to SetAllowedInputMethods, which I think is more
consistent with other parts.  (Please update the comment, too, if you're going
to rename this.)

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/in...
ui/base/ime/chromeos/input_method_manager.h:227:
GetAllowedKeyboardLayoutInputMethods() = 0;
nit: Ditto.

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_input_method_manager.h (right):

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_input_method_manager.h:65: std::vector<std::string>
allowed_keyboard_layout_input_method_ids;
nit: I don't see a reason to expose this member as public:.  Should be private:,
I guess?  (Please rename this with the underscore at the end of the name if
you're going to move this to private:.)

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_input_method_manager.h:65: std::vector<std::string>
allowed_keyboard_layout_input_method_ids;
nit: I'd recommend to rename this without "keyboard_layout_".

[pmarko, 2017-02-01 10:04:12]

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/in...
File ui/base/ime/chromeos/input_method_manager.h (right):

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/in...
ui/base/ime/chromeos/input_method_manager.h:221: virtual bool
SetAllowedKeyboardLayoutInputMethods(
On 2017/01/31 15:18:29, Yuki wrote:
> nit: "keyboard layout input method" sounds a bit weird to me because "input
> method" in a concept containing direct input (with a variety of keyboard
> layouts) and non-direct input (such as Chinese/Japanese/Korean).
> 
> I'd recommend to rename this to SetAllowedInputMethods, which I think is more
> consistent with other parts.  (Please update the comment, too, if you're going
> to rename this.)

Good point, the naming indeed is a little weird.

My idea was to actually only restrict physical keyboard layouts (i.e. don't stop
people from using some IME extension they might want), that's why I included
"KeyboardLayout" in there.
That's also why InputMethodManagerImpl::IsInputMethodAllowed checks for
IsKeyboardLayout.

Currently, this should be irrelevant, as it's only used on the login screen, but
if we ever extend the policy to user sessions, I didn't want to restrict
extensions which the user might need e.g. for accessibility.

Would you be fine with SetAllowedKeyboardLayouts, i.e. strip the InputMethod
part as it is redundant?
Or do you feel that restricting xkb keyboard layouts only is a dubious goal and
we should just treat all input methods the same way?

[Yuki, 2017-02-01 12:12:20]

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/in...
File ui/base/ime/chromeos/input_method_manager.h (right):

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/in...
ui/base/ime/chromeos/input_method_manager.h:221: virtual bool
SetAllowedKeyboardLayoutInputMethods(
On 2017/02/01 10:04:12, pmarko wrote:
> On 2017/01/31 15:18:29, Yuki wrote:
> > nit: "keyboard layout input method" sounds a bit weird to me because "input
> > method" in a concept containing direct input (with a variety of keyboard
> > layouts) and non-direct input (such as Chinese/Japanese/Korean).
> > 
> > I'd recommend to rename this to SetAllowedInputMethods, which I think is
more
> > consistent with other parts.  (Please update the comment, too, if you're
going
> > to rename this.)
> 
> Good point, the naming indeed is a little weird.
> 
> My idea was to actually only restrict physical keyboard layouts (i.e. don't
stop
> people from using some IME extension they might want), that's why I included
> "KeyboardLayout" in there.
> That's also why InputMethodManagerImpl::IsInputMethodAllowed checks for
> IsKeyboardLayout.
> 
> Currently, this should be irrelevant, as it's only used on the login screen,
but
> if we ever extend the policy to user sessions, I didn't want to restrict
> extensions which the user might need e.g. for accessibility.
> 
> Would you be fine with SetAllowedKeyboardLayouts, i.e. strip the InputMethod
> part as it is redundant?
> Or do you feel that restricting xkb keyboard layouts only is a dubious goal
and
> we should just treat all input methods the same way?

My personal preference is to use a general tool with a specific policy. 
Ideally, "policy" should be injected as config data, etc., and better to avoid
hard-coding the policy.  Otherwise, if the policy changed, we would need to
change the design drastically.  I'd recommend to make this interface not depend
on a specific policy, and to implement a specific implementation class (i.e.
InputMethodManagerImpl::StateImpl) with a specific policy.

[pmarko, 2017-02-01 14:07:57]

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/in...
File ui/base/ime/chromeos/input_method_manager.h (right):

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/in...
ui/base/ime/chromeos/input_method_manager.h:221: virtual bool
SetAllowedKeyboardLayoutInputMethods(
On 2017/02/01 12:12:19, Yuki wrote:
> On 2017/02/01 10:04:12, pmarko wrote:
> > On 2017/01/31 15:18:29, Yuki wrote:
> > > nit: "keyboard layout input method" sounds a bit weird to me because
"input
> > > method" in a concept containing direct input (with a variety of keyboard
> > > layouts) and non-direct input (such as Chinese/Japanese/Korean).
> > > 
> > > I'd recommend to rename this to SetAllowedInputMethods, which I think is
> more
> > > consistent with other parts.  (Please update the comment, too, if you're
> going
> > > to rename this.)
> > 
> > Good point, the naming indeed is a little weird.
> > 
> > My idea was to actually only restrict physical keyboard layouts (i.e. don't
> stop
> > people from using some IME extension they might want), that's why I included
> > "KeyboardLayout" in there.
> > That's also why InputMethodManagerImpl::IsInputMethodAllowed checks for
> > IsKeyboardLayout.
> > 
> > Currently, this should be irrelevant, as it's only used on the login screen,
> but
> > if we ever extend the policy to user sessions, I didn't want to restrict
> > extensions which the user might need e.g. for accessibility.
> > 
> > Would you be fine with SetAllowedKeyboardLayouts, i.e. strip the InputMethod
> > part as it is redundant?
> > Or do you feel that restricting xkb keyboard layouts only is a dubious goal
> and
> > we should just treat all input methods the same way?
> 
> My personal preference is to use a general tool with a specific policy. 
> Ideally, "policy" should be injected as config data, etc., and better to avoid
> hard-coding the policy.  Otherwise, if the policy changed, we would need to
> change the design drastically.  I'd recommend to make this interface not
depend
> on a specific policy, and to implement a specific implementation class (i.e.
> InputMethodManagerImpl::StateImpl) with a specific policy.

Done.

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/in...
ui/base/ime/chromeos/input_method_manager.h:227:
GetAllowedKeyboardLayoutInputMethods() = 0;
On 2017/01/31 15:18:29, Yuki wrote:
> nit: Ditto.

Done.

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_input_method_manager.h (right):

https://codereview.chromium.org/2652793003/diff/80001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_input_method_manager.h:65: std::vector<std::string>
allowed_keyboard_layout_input_method_ids;
On 2017/01/31 15:18:29, Yuki wrote:
> nit: I'd recommend to rename this without "keyboard_layout_".

Done (both).

[pmarko, 2017-02-06 08:48:06]

pmarko@chromium.org changed reviewers:
- shuchen@chromium.org

[Shu Chen, 2017-02-07 02:47:08]

shuchen@chromium.org changed reviewers:
+ shuchen@chromium.org

[Shu Chen, 2017-02-07 02:47:09]

https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:283:
IsInputMethodAllowed(candidate))
nit: add brackets {} for the following line.

https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:239: return;
nit: wrap this by brackets {}.

https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:252: }
nit: no need these brackets.

https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:354:
StopEnforcingPolicyInputMethods();
I'd like to understand about what will happen after this?
By the implementation of InputMethodMangerImpl, SetAllowedInputMethods(<empty>)
won't make any impact to the input method list.
If that is intentional, why we need to StopEnforcingPolicyInputMethods?

[pmarko, 2017-02-07 10:44:10]

Thank you for taking the time to review this CL after your vacation! Please see
my comments below.

https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:283:
IsInputMethodAllowed(candidate))
On 2017/02/07 02:47:09, Shu Chen wrote:
> nit: add brackets {} for the following line.

Done.

https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:239: return;
On 2017/02/07 02:47:09, Shu Chen wrote:
> nit: wrap this by brackets {}.

Done.

https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:252: }
On 2017/02/07 02:47:09, Shu Chen wrote:
> nit: no need these brackets.

Done.

https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:354:
StopEnforcingPolicyInputMethods();
On 2017/02/07 02:47:09, Shu Chen wrote:
> I'd like to understand about what will happen after this?
> By the implementation of InputMethodMangerImpl,
SetAllowedInputMethods(<empty>)
> won't make any impact to the input method list.
> If that is intentional, why we need to StopEnforcingPolicyInputMethods?

After SetAllowedInputMethods(non_empty_vec), additional input methods can not be
enabled. Calling SetAllowedInputMethods(empty_vec) will not change the input
method list, but it will be possible to add additional input methods again
(using EnableInputMethod).

This is meant for consistency: If the InputMethodManager's State outlives
SigninScreenHandler, I think SigninScreenHandler should not leave input method
restrictions in place. That's why these are removed in the destructor.

[Shu Chen, 2017-02-07 13:33:41]

On 2017/02/07 10:44:10, pmarko wrote:
> Thank you for taking the time to review this CL after your vacation! Please
see
> my comments below.
> 
>
https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/chromeo...
> File chrome/browser/chromeos/input_method/input_method_manager_impl.cc
(right):
> 
>
https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/chromeo...
> chrome/browser/chromeos/input_method/input_method_manager_impl.cc:283:
> IsInputMethodAllowed(candidate))
> On 2017/02/07 02:47:09, Shu Chen wrote:
> > nit: add brackets {} for the following line.
> 
> Done.
> 
>
https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/ui/webu...
> File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):
> 
>
https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:239: return;
> On 2017/02/07 02:47:09, Shu Chen wrote:
> > nit: wrap this by brackets {}.
> 
> Done.
> 
>
https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:252: }
> On 2017/02/07 02:47:09, Shu Chen wrote:
> > nit: no need these brackets.
> 
> Done.
> 
>
https://codereview.chromium.org/2652793003/diff/140001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:354:
> StopEnforcingPolicyInputMethods();
> On 2017/02/07 02:47:09, Shu Chen wrote:
> > I'd like to understand about what will happen after this?
> > By the implementation of InputMethodMangerImpl,
> SetAllowedInputMethods(<empty>)
> > won't make any impact to the input method list.
> > If that is intentional, why we need to StopEnforcingPolicyInputMethods?
> 
> After SetAllowedInputMethods(non_empty_vec), additional input methods can not
be
> enabled. Calling SetAllowedInputMethods(empty_vec) will not change the input
> method list, but it will be possible to add additional input methods again
> (using EnableInputMethod).
> 
> This is meant for consistency: If the InputMethodManager's State outlives
> SigninScreenHandler, I think SigninScreenHandler should not leave input method
> restrictions in place. That's why these are removed in the destructor.

Thanks for the clarification, lgtm.

[Nico, 2017-02-07 15:14:22]

application_lifetime lgtm

https://codereview.chromium.org/2652793003/diff/160001/chrome/browser/lifetim...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/2652793003/diff/160001/chrome/browser/lifetim...
chrome/browser/lifetime/application_lifetime.cc:116: #endif
this does #if cros #endif #if cros #endif -- remove inner #endif #if?

[pmarko, 2017-02-08 07:02:03]

https://codereview.chromium.org/2652793003/diff/160001/chrome/browser/lifetim...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/2652793003/diff/160001/chrome/browser/lifetim...
chrome/browser/lifetime/application_lifetime.cc:116: #endif
On 2017/02/07 15:14:22, Nico wrote:
> this does #if cros #endif #if cros #endif -- remove inner #endif #if?

Done.

[pmarko, 2017-02-08 11:04:45]

The CQ bit was checked by pmarko@chromium.org

[pmarko, 2017-02-08 11:04:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, skuhne@chromium.org, xiyuan@chromium.org,
pastarmovj@chromium.org, yukishiino@chromium.org, haraken@chromium.org,
thakis@chromium.org, shuchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2652793003/#ps200001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-02-08 11:04:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 12:18:40]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1486551885834030,
"parent_rev": "691a5713a30074cbd2d6d3abe0a32c0cc92c8378", "commit_rev":
"8c3ffb5e698796da543bcbf2192bbb5aebb90807"}

[commit-bot: I haz the power, 2017-02-08 12:19:21]

Description was changed from

==========
Add login screen locale and input method device policies

This CL introduces two device policies for Chrome OS:
+ DeviceLoginScreenLocales
  Mandates a specific locale for the login screen.
  (Note that the policy value is a list for future compatibility, in
  case dynamically switching login screen locales is implemented in the
  future. Currently only the first value is used).
+ DeviceLoginScreenInputMethods
  Enforces input methods on the login screen.

This is done by:
Locale:
- Introducing cros setting kDeviceLoginScreenLocales, controlled by the new
policy
- Applying Login Screen locale before login screen is displayed
- Pre-setting kApplicationLocale on application exit for the next run

Input Methods:
- Introducing cros setting kDeviceLoginScreenInputMethods, controlled by the new
policy
- new method SetAllowedKeyboardLayoutInputMethods on InputMethodManager to
restrict input methods
- enforcing keyboard layouts (through InputMethodManager) on sign-in screen
(depending on which user's fod is focused)
- adding info to the tray icon to display that input methods are controlled by
policy

See go/cros-locale-input-policy-dd for more detail.

BUG=373324
TEST=New browser_tests, new unit_tests, manual test with YAPS.
==========

to

==========
Add login screen locale and input method device policies

This CL introduces two device policies for Chrome OS:
+ DeviceLoginScreenLocales
  Mandates a specific locale for the login screen.
  (Note that the policy value is a list for future compatibility, in
  case dynamically switching login screen locales is implemented in the
  future. Currently only the first value is used).
+ DeviceLoginScreenInputMethods
  Enforces input methods on the login screen.

This is done by:
Locale:
- Introducing cros setting kDeviceLoginScreenLocales, controlled by the new
policy
- Applying Login Screen locale before login screen is displayed
- Pre-setting kApplicationLocale on application exit for the next run

Input Methods:
- Introducing cros setting kDeviceLoginScreenInputMethods, controlled by the new
policy
- new method SetAllowedKeyboardLayoutInputMethods on InputMethodManager to
restrict input methods
- enforcing keyboard layouts (through InputMethodManager) on sign-in screen
(depending on which user's fod is focused)
- adding info to the tray icon to display that input methods are controlled by
policy

See go/cros-locale-input-policy-dd for more detail.

BUG=373324
TEST=New browser_tests, new unit_tests, manual test with YAPS.

Review-Url: https://codereview.chromium.org/2652793003
Cr-Commit-Position: refs/heads/master@{#448961}
Committed:
https://chromium.googlesource.com/chromium/src/+/8c3ffb5e698796da543bcbf2192b...
==========

[commit-bot: I haz the power, 2017-02-08 12:19:25]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/8c3ffb5e698796da543bcbf2192b...

[Thiemo Nagel, 2017-02-13 11:33:17]

tnagel@chromium.org changed reviewers:
+ tnagel@chromium.org

[Thiemo Nagel, 2017-02-13 11:33:20]

https://codereview.chromium.org/2652793003/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/2652793003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:757: // A list
of allowed locales on the login page.
Nit: login screen

https://codereview.chromium.org/2652793003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:762: // A list
of allowed input methods on the login page.
Nit: login screen

[pmarko, 2017-02-13 17:50:31]

https://codereview.chromium.org/2652793003/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/2652793003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:757: // A list
of allowed locales on the login page.
On 2017/02/13 11:33:20, Thiemo Nagel (slow) wrote:
> Nit: login screen

You're right. Mini-CL to correct this:
https://codereview.chromium.org/2694833002 .

https://codereview.chromium.org/2652793003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:762: // A list
of allowed input methods on the login page.
On 2017/02/13 11:33:20, Thiemo Nagel (slow) wrote:
> Nit: login screen
See above.