[ChromeOS] Expose keyboard remapping to signin screen.

[ChromeOS] Expose keyboard remapping to signin screen.

For users that have changed keyboard mappings in settings, expose those
mappings on the signin screen. It might benefit use cases below:
- User switches Ctrl and Alt, uses Alt+A as Select-All to delete a typo-ed password
- User switches Search to Caps-Lock, uses Caps-Lock when typing password

BUG=651905
Review-Url: https://codereview.chromium.org/2702393003
Cr-Commit-Position: refs/heads/master@{#452890}
Committed: https://chromium.googlesource.com/chromium/src/+/e6172c99f6d2ed7f1fe9a0ae26e3d836d6c931ac

[xdai1, 2017-02-22 00:12:01]

Patchset #1 (id:1) has been deleted

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

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

[xdai1, 2017-02-22 00:17:02]

xiyuan@, kpschoedel@, could you help review this CL please? Thanks!
xiyuan@: all
kpschoedel@: event_rewriter.cc

[xdai1, 2017-02-22 00:18:13]

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

[commit-bot: I haz the power, 2017-02-22 00:20:23]

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-22 00:41:21]

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

[Alexander Alekseev, 2017-02-22 00:41:22]

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

https://codereview.chromium.org/2702393003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:976:
registry->RegisterStringPref(prefs::kFocusedPodUserId, std::string());
Could you also explain the idea behind this?
CL description does not say anything about it.

[xdai1, 2017-02-22 00:52:10]

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

https://codereview.chromium.org/2702393003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:976:
registry->RegisterStringPref(prefs::kFocusedPodUserId, std::string());
On 2017/02/22 00:41:22, Alexander Alekseev wrote:
> Could you also explain the idea behind this?
> CL description does not say anything about it.

kFocusedPodUserId is used to track the current focused user pod in the login
screen. It then will be used in GetRemappedKey() function in event_rewriter.cc
to get the mapping preference of the current focused user.

[Alexander Alekseev, 2017-02-22 01:04:47]

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

https://codereview.chromium.org/2702393003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:976:
registry->RegisterStringPref(prefs::kFocusedPodUserId, std::string());
On 2017/02/22 00:52:09, xdai1 wrote:
> On 2017/02/22 00:41:22, Alexander Alekseev wrote:
> > Could you also explain the idea behind this?
> > CL description does not say anything about it.
> 
> kFocusedPodUserId is used to track the current focused user pod in the login
> screen. It then will be used in GetRemappedKey() function in event_rewriter.cc
> to get the mapping preference of the current focused user.

It seems to me that storing temporary (not statefull) data to local state is not
nice. Could you find another way of doing this?

For example, we can use OobeUI (through
LoginDisplayHost::default_host()->GetOobeUI() ) for that.

[xdai1, 2017-02-22 01:40:36]

Patchset #2 (id:40001) has been deleted

[xdai1, 2017-02-22 01:42:16]

alemate@, I've addressed your comment. Please take another look. Thanks.

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

https://codereview.chromium.org/2702393003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:976:
registry->RegisterStringPref(prefs::kFocusedPodUserId, std::string());
On 2017/02/22 01:04:47, Alexander Alekseev wrote:
> On 2017/02/22 00:52:09, xdai1 wrote:
> > On 2017/02/22 00:41:22, Alexander Alekseev wrote:
> > > Could you also explain the idea behind this?
> > > CL description does not say anything about it.
> > 
> > kFocusedPodUserId is used to track the current focused user pod in the login
> > screen. It then will be used in GetRemappedKey() function in
event_rewriter.cc
> > to get the mapping preference of the current focused user.
> 
> It seems to me that storing temporary (not statefull) data to local state is
not
> nice. Could you find another way of doing this?
> 
> For example, we can use OobeUI (through
> LoginDisplayHost::default_host()->GetOobeUI() ) for that.

Thanks. Done.

[Alexander Alekseev, 2017-02-22 10:51:50]

https://codereview.chromium.org/2702393003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/2702393003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter.cc:148: &value);
I am not an owner of this file, but it seems to me that this is too
login-specific for event rewriter.

Did you consider moving these new lines to:
bool SigninScreenHandler::GetKeyboardRemappingValue(
    const std::string& pref_name, 
    int* value);
?

It could return true if there is a focused pod and a preference for focused
user.

Then, it will become:

if (!LoginDisplayHost::default_host() ||
    !LoginDisplayHost::default_host()>GetOobeUI()
        ->signin_screen_handler()
        ->GetKeyboardRemappingValue(pref_name, &value)) {

  if (!pref_service.FindPreference(pref_name.c_str()))
    return NULL;  // The |pref_name| hasn't been registered.

  const int value = pref_service.GetInteger(pref_name.c_str());
}

for(...) {
}

return nullptr;
}

[xiyuan, 2017-02-22 17:40:18]

https://codereview.chromium.org/2702393003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/2702393003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter.cc:146: if
(!focused_account_id.empty()) {
nit: empty() -> is_valid()

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

https://codereview.chromium.org/2702393003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:273: const
AccountId& focused_pod_account_id() {
nit: Make it a const method
i.e.
  const AccountId& focused_pod_account_id() const {

https://codereview.chromium.org/2702393003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:274: return
*focused_pod_account_id_.get();
What if |focused_pod_account_id_| holds nullptr and does not own anything?

[xdai1, 2017-02-22 18:36:17]

xiyuan@, alemate@, please take another look, thanks for the review!

https://codereview.chromium.org/2702393003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/2702393003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter.cc:148: &value);
On 2017/02/22 10:51:50, Alexander Alekseev wrote:
> I am not an owner of this file, but it seems to me that this is too
> login-specific for event rewriter.
> 
> Did you consider moving these new lines to:
> bool SigninScreenHandler::GetKeyboardRemappingValue(
>     const std::string& pref_name, 
>     int* value);
> ?
> 
> It could return true if there is a focused pod and a preference for focused
> user.
> 
> Then, it will become:
> 
> if (!LoginDisplayHost::default_host() ||
>     !LoginDisplayHost::default_host()>GetOobeUI()
>         ->signin_screen_handler()
>         ->GetKeyboardRemappingValue(pref_name, &value)) {
> 
>   if (!pref_service.FindPreference(pref_name.c_str()))
>     return NULL;  // The |pref_name| hasn't been registered.
> 
>   const int value = pref_service.GetInteger(pref_name.c_str());
> }
> 
> for(...) {
> }
> 
> return nullptr;
> }

Done.

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

https://codereview.chromium.org/2702393003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:273: const
AccountId& focused_pod_account_id() {
On 2017/02/22 17:40:18, xiyuan wrote:
> nit: Make it a const method
> i.e.
>   const AccountId& focused_pod_account_id() const {

Removed this function according to alemate@'s suggestion.

[xiyuan, 2017-02-22 18:50:58]

https://codereview.chromium.org/2702393003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/2702393003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter.cc:147: else
nit: this "else" can be removed.

https://codereview.chromium.org/2702393003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/2702393003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/preferences.cc:769: }
Can we use a list and a loop instead?

e.g.

const char* const kLanguageRemapPrefs[] = {
  prefs::kLanguageRemapSearchKeyTo,
  ...
};

for (auto remap_pref : kLanguageRemapPrefs) {
  if (pref_name == remap_pref ||
      reason != REASON_ACTIVE_USER_CHANGED) {
   ...
  }
}

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

https://codereview.chromium.org/2702393003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:686: return
true;
nit: get rid of "if" ?

return focused_pod_account_id_ && focused_pod_account_id_->is_valid() &&
    user_manager::known_user::GetIntegerPref(*focused_pod_account_id_,
                                             pref_name, value));

[xdai1, 2017-02-22 19:26:30]

xiyuan@, please take another look. thanks for the review!

https://codereview.chromium.org/2702393003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/2702393003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter.cc:147: else
On 2017/02/22 18:50:58, xiyuan wrote:
> nit: this "else" can be removed.

Done.

https://codereview.chromium.org/2702393003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/2702393003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/preferences.cc:769: }
On 2017/02/22 18:50:58, xiyuan wrote:
> Can we use a list and a loop instead?
> 
> e.g.
> 
> const char* const kLanguageRemapPrefs[] = {
>   prefs::kLanguageRemapSearchKeyTo,
>   ...
> };
> 
> for (auto remap_pref : kLanguageRemapPrefs) {
>   if (pref_name == remap_pref ||
>       reason != REASON_ACTIVE_USER_CHANGED) {
>    ...
>   }
> }

Done.

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

https://codereview.chromium.org/2702393003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:686: return
true;
On 2017/02/22 18:50:58, xiyuan wrote:
> nit: get rid of "if" ?
> 
> return focused_pod_account_id_ && focused_pod_account_id_->is_valid() &&
>     user_manager::known_user::GetIntegerPref(*focused_pod_account_id_,
>                                              pref_name, value));

Done.

[xiyuan, 2017-02-22 20:13:47]

lgtm

[xdai1, 2017-02-23 20:05:37]

kindly ping?

[Alexander Alekseev, 2017-02-24 09:36:06]

lgtm

[xdai1, 2017-02-24 18:28:36]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2017-02-24 18:29:08]

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-24 19:01:55]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487960916880870,
"parent_rev": "c702a811babe12110a84d644ee81b6d99b2f4716", "commit_rev":
"e6172c99f6d2ed7f1fe9a0ae26e3d836d6c931ac"}

[commit-bot: I haz the power, 2017-02-24 19:05:18]

Description was changed from

==========
[ChromeOS] Expose keyboard remapping to signin screen.

For users that have changed keyboard mappings in settings, expose those
mappings on the signin screen. It might benefit use cases below:
- User switches Ctrl and Alt, uses Alt+A as Select-All to delete a typo-ed
password
- User switches Search to Caps-Lock, uses Caps-Lock when typing password

BUG=651905
==========

to

==========
[ChromeOS] Expose keyboard remapping to signin screen.

For users that have changed keyboard mappings in settings, expose those
mappings on the signin screen. It might benefit use cases below:
- User switches Ctrl and Alt, uses Alt+A as Select-All to delete a typo-ed
password
- User switches Search to Caps-Lock, uses Caps-Lock when typing password

BUG=651905

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

[commit-bot: I haz the power, 2017-02-24 19:05:19]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/e6172c99f6d2ed7f1fe9a0ae26e3...