[Chrome OS] Expose keyboard auto repeat delay/interval to login screen.

[Chrome OS] Expose keyboard auto repeat delay/interval to login screen.

BUG=673901
TEST=manually

Review-Url: https://codereview.chromium.org/2697413003
Cr-Commit-Position: refs/heads/master@{#451477}
Committed: https://chromium.googlesource.com/chromium/src/+/c97cc0aacb1a2940f798715c6538b6fc43ee5243

[xdai1, 2017-02-17 01:15:53]

xdai@chromium.org changed reviewers:
+ xiyuan@chromium.org

[xdai1, 2017-02-17 01:15:54]

xiyuan@, could you help review this CL please? Thanks!

[xdai1, 2017-02-17 01:16:00]

The CQ bit was checked by xdai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 01:17:46]

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

[Alexander Alekseev, 2017-02-17 02:18:22]

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

[Alexander Alekseev, 2017-02-17 02:18:24]

https://codereview.chromium.org/2697413003/diff/1/chrome/browser/chromeos/pre...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/2697413003/diff/1/chrome/browser/chromeos/pre...
chrome/browser/chromeos/preferences.cc:802:
user_->GetAccountId().GetUserEmail(), rate.initial_delay_in_ms);
e-mail as storage key is deprecated.
Could you use GetGaiaIdKey() for that?

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

https://codereview.chromium.org/2697413003/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1619: void
SigninScreenHandler::SetKeyboardSettings(const std::string& username) {
AccountId ?

https://codereview.chromium.org/2697413003/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

https://codereview.chromium.org/2697413003/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:445: void
SetKeyboardSettings(const std::string& username);
AccountId ?

[commit-bot: I haz the power, 2017-02-17 03:20:32]

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

[commit-bot: I haz the power, 2017-02-17 03:20:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[xiyuan, 2017-02-17 17:08:15]

We have stored per-user global data in a dictionary like this before. IMO, this
is not right and we have handle each of them separately, esp. when removing a
user. We should probably have a "global_user_prefs" dict in Local State and each
user has a sub dict in it for its prefs.

i.e

"global_user_prefs": {
  "user_A_account_id_key" : {
     "pref_name": pref_value,
      ...
  },
  "user_B_account_id_key" : {
     "pref_name": pref_value,
      ...
  },
}

It is not caused by this CL. But it would be great that we can start to migrate
to this setup.

Besides that, I think we should make get/set user keyboard prefs part of
UserManager interface, similar to SaveUserOAuthStatus/LoadUserOAuthStatus. Also
update UserManagerBase::RemoveNonCryptohomeData to remove it when user is
removed from the system.

https://codereview.chromium.org/2697413003/diff/1/chrome/browser/chromeos/pre...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/2697413003/diff/1/chrome/browser/chromeos/pre...
chrome/browser/chromeos/preferences.cc:802:
user_->GetAccountId().GetUserEmail(), rate.initial_delay_in_ms);
On 2017/02/17 02:18:23, Alexander Alekseev wrote:
> e-mail as storage key is deprecated.
> Could you use GetGaiaIdKey() for that?

Did you mean AccountId::GetAccountIdKey()? If so, do we need to worry about
legacy users (i.e. AccountType::UNKNOWN)?

[Alexander Alekseev, 2017-02-17 22:19:05]

On 2017/02/17 17:08:15, xiyuan wrote:
> We have stored per-user global data in a dictionary like this before. IMO,
this
> is not right and we have handle each of them separately, esp. when removing a
> user. We should probably have a "global_user_prefs" dict in Local State and
each
> user has a sub dict in it for its prefs.
> 
> i.e
> 
> "global_user_prefs": {
>   "user_A_account_id_key" : {
>      "pref_name": pref_value,
>       ...
>   },
>   "user_B_account_id_key" : {
>      "pref_name": pref_value,
>       ...
>   },
> }
> 
> It is not caused by this CL. But it would be great that we can start to
migrate
> to this setup.

There is already known_users dictionary. See user_manager::known_user.h

> 
> Besides that, I think we should make get/set user keyboard prefs part of
> UserManager interface, similar to SaveUserOAuthStatus/LoadUserOAuthStatus.
Also
> update UserManagerBase::RemoveNonCryptohomeData to remove it when user is
> removed from the system.
> 
>
https://codereview.chromium.org/2697413003/diff/1/chrome/browser/chromeos/pre...
> File chrome/browser/chromeos/preferences.cc (right):
> 
>
https://codereview.chromium.org/2697413003/diff/1/chrome/browser/chromeos/pre...
> chrome/browser/chromeos/preferences.cc:802:
> user_->GetAccountId().GetUserEmail(), rate.initial_delay_in_ms);
> On 2017/02/17 02:18:23, Alexander Alekseev wrote:
> > e-mail as storage key is deprecated.
> > Could you use GetGaiaIdKey() for that?
> 
> Did you mean AccountId::GetAccountIdKey()? If so, do we need to worry about
> legacy users (i.e. AccountType::UNKNOWN)?

[Alexander Alekseev, 2017-02-17 22:40:59]

On 2017/02/17 22:19:05, Alexander Alekseev wrote:
> On 2017/02/17 17:08:15, xiyuan wrote:
>
https://codereview.chromium.org/2697413003/diff/1/chrome/browser/chromeos/pre...
> > chrome/browser/chromeos/preferences.cc:802:
> > user_->GetAccountId().GetUserEmail(), rate.initial_delay_in_ms);
> > On 2017/02/17 02:18:23, Alexander Alekseev wrote:
> > > e-mail as storage key is deprecated.
> > > Could you use GetGaiaIdKey() for that?
> > 
> > Did you mean AccountId::GetAccountIdKey()? If so, do we need to worry about
> > legacy users (i.e. AccountType::UNKNOWN)?


GaiaId is remembered in known_users for all users on signin since M43 (stable
May 2015).

So we are talking about users:
1) Not having GaiaID (supervised)
2) Not logged in since May 2015
3) Not updated thereir ChromeOS since.

I believe we could ignore them, but using known_users will solve this problem
too ;)

[xdai1, 2017-02-17 23:53:27]

xiyuan@, alemate@, please take another look at this CL, thanks! Seems in this
way I don't need to move the get/set keyboard prefs to UserManager. The keyboard
prefs will be removed by known_user::RemovePrefs() when the user is removed (see
https://cs.chromium.org/chromium/src/components/user_manager/user_manager_bas...).

[xdai1, 2017-02-17 23:54:49]

The CQ bit was checked by xdai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 23:55:42]

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

[xiyuan, 2017-02-17 23:58:09]

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

https://codereview.chromium.org/2697413003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1624:
->SetAutoRepeatEnabled(false);
Do we need to do this when the pref is not found?

[xdai1, 2017-02-18 00:12:57]

Please take another look, thanks!

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

https://codereview.chromium.org/2697413003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1624:
->SetAutoRepeatEnabled(false);
On 2017/02/17 23:58:09, xiyuan wrote:
> Do we need to do this when the pref is not found?

|auto_repeat_enabled|, |auto_repeat_delay| and |auto_repeat_interval| are all
initialized to their default values. If we can't find the values in the pref, we
just fall back to the default behavior, which is the same behavior before this
change. So I think it should be fine?

[xiyuan, 2017-02-18 00:15:53]

lgtm

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

https://codereview.chromium.org/2697413003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1624:
->SetAutoRepeatEnabled(false);
On 2017/02/18 00:12:57, xdai1 wrote:
> On 2017/02/17 23:58:09, xiyuan wrote:
> > Do we need to do this when the pref is not found?
> 
> |auto_repeat_enabled|, |auto_repeat_delay| and |auto_repeat_interval| are all
> initialized to their default values. If we can't find the values in the pref,
we
> just fall back to the default behavior, which is the same behavior before this
> change. So I think it should be fine?

Acknowledged.

[Alexander Alekseev, 2017-02-18 00:18:36]

lgtm

[xdai1, 2017-02-18 00:39:41]

The CQ bit was checked by xdai@chromium.org

[xdai1, 2017-02-18 00:39:42]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
alemate@chromium.org
Link to the patchset: https://codereview.chromium.org/2697413003/#ps40001
(title: "Nit.")

[commit-bot: I haz the power, 2017-02-18 00:40:48]

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-18 02:43:33]

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

[commit-bot: I haz the power, 2017-02-18 02:43:34]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[xdai1, 2017-02-18 18:06:13]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2017-02-18 18:06:22]

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-18 18:10:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487441173365940,
"parent_rev": "8fc480797adb8b57f4dada238f2d85c0fe42115b", "commit_rev":
"c97cc0aacb1a2940f798715c6538b6fc43ee5243"}

[commit-bot: I haz the power, 2017-02-18 18:10:49]

Description was changed from

==========
[Chrome OS] Expose keyboard auto repeat delay/interval to login screen.

BUG=673901
TEST=manually
==========

to

==========
[Chrome OS] Expose keyboard auto repeat delay/interval to login screen.

BUG=673901
TEST=manually

Review-Url: https://codereview.chromium.org/2697413003
Cr-Commit-Position: refs/heads/master@{#451477}
Committed:
https://chromium.googlesource.com/chromium/src/+/c97cc0aacb1a2940f798715c6538...
==========

[commit-bot: I haz the power, 2017-02-18 18:10:50]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c97cc0aacb1a2940f798715c6538...