Guest Mode: input method should default to the underlying latin keyboard layout.

Guest Mode: input method should default to the underlying latin keyboard layout.

Currently Guest mode always uses default value of
Preferences.PreferredInputMethod = "" (empty string) that
always leads to "us" keyboard layout (compiled-in default).

Guest sessions should have the following input methods/keyboards readily
available:
 - the VPD value (assuming the latin keyboard representing the underlying
   hardware; should be the default; e.g. French chromebook => azerty)
 - the additional input method/keyboard based on the locale (e.g. Japanese
   locale => should also add the Japanese IME; there are existing rules for
   this)


BUG=329018
TEST=unittest

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

[Alexander Alekseev, 2014-01-17 21:05:40]

Please review.

[Alexander Alekseev, 2014-01-20 13:43:30]

Please review:

komatsu@   : input_method/*
nkostylev@ : all the other files.

[Hiro Komatsu, 2014-01-21 05:46:00]

I am afraid to use the owner's IME settings to guest users.  It makes
complexity.  Plus IMEs implemented as extensions cannot work in this way.

If using VPD and locale covers almost all case, I want to keep it as is.

Kenji, Nona, what do you think about it?

https://codereview.chromium.org/133273032/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/133273032/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:572:
input_method::InputMethodManager* const ime =
Please use 'imm' or 'ime_manager' instead of 'ime'.

https://codereview.chromium.org/133273032/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/133273032/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.cc:633: if
(!InputMethodManager::Get()->IsLoginKeyboard(input_method_id)) {
Please check the actual implementation of IsLogingKeyboard.  You do not need to
include input_method_manager.h.

https://codereview.chromium.org/133273032/diff/190001/chromeos/ime/input_meth...
File chromeos/ime/input_method_manager.h (right):

https://codereview.chromium.org/133273032/diff/190001/chromeos/ime/input_meth...
chromeos/ime/input_method_manager.h:121: virtual void EnableLayouts(const
std::string& language_code,
To add keyboard layouts after login, please use EnableInputMethod(s).

[kenjibaheux, 2014-01-21 06:00:54]

> I am afraid to use the owner's IME settings to guest users.  It makes
> complexity.  Plus IMEs implemented as extensions cannot work in this way.
> 
> If using VPD and locale covers almost all case, I want to keep it as is.
> 
> Kenji, Nona, what do you think about it?

I think we can drop this last part.
As I commented on the bug it's actually more trouble than it's worth so let's
keep the change to VPD and locale based layouts.

[Alexander Alekseev, 2014-01-21 14:05:26]

https://codereview.chromium.org/133273032/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/133273032/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:572:
input_method::InputMethodManager* const ime =
On 2014/01/21 05:46:01, Hiro Komatsu wrote:
> Please use 'imm' or 'ime_manager' instead of 'ime'.

Done.

https://codereview.chromium.org/133273032/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/133273032/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.cc:633: if
(!InputMethodManager::Get()->IsLoginKeyboard(input_method_id)) {
On 2014/01/21 05:46:01, Hiro Komatsu wrote:
> Please check the actual implementation of IsLogingKeyboard.  You do not need
to
> include input_method_manager.h.

Done.

https://codereview.chromium.org/133273032/diff/190001/chromeos/ime/input_meth...
File chromeos/ime/input_method_manager.h (right):

https://codereview.chromium.org/133273032/diff/190001/chromeos/ime/input_meth...
chromeos/ime/input_method_manager.h:121: virtual void EnableLayouts(const
std::string& language_code,
On 2014/01/21 05:46:01, Hiro Komatsu wrote:
> To add keyboard layouts after login, please use EnableInputMethod(s).

What do you mean? This method has always been used for loading locale layouts
after locale switch. It happens only once after user logs in and before browser
starts.

EnableInputMethod(s) doesn't accept locale.

[Hiro Komatsu, 2014-01-22 10:01:55]

https://codereview.chromium.org/133273032/diff/190001/chromeos/ime/input_meth...
File chromeos/ime/input_method_manager.h (right):

https://codereview.chromium.org/133273032/diff/190001/chromeos/ime/input_meth...
chromeos/ime/input_method_manager.h:121: virtual void EnableLayouts(const
std::string& language_code,
On 2014/01/21 14:05:27, alemate wrote:
> On 2014/01/21 05:46:01, Hiro Komatsu wrote:
> > To add keyboard layouts after login, please use EnableInputMethod(s).
> 
> What do you mean? This method has always been used for loading locale layouts
> after locale switch. It happens only once after user logs in and before
browser
> starts.
> 
> EnableInputMethod(s) doesn't accept locale.

InputMethodUtil::GetInputMethodIdsFromLanguageCode converts language_code to
input_method_id.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...


I prefer to use the above function in the caller or add a new function (if
necessary), rather than a boolean flag of login_layouts_only in this case.

https://codereview.chromium.org/133273032/diff/250001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.h (right):

https://codereview.chromium.org/133273032/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.h:107: bool
IsLoginKeyboard(const std::string& layout) const;
Please add comment and unittest.

layout -> input_method_id.

[Alexander Alekseev, 2014-01-22 11:35:05]

https://codereview.chromium.org/133273032/diff/190001/chromeos/ime/input_meth...
File chromeos/ime/input_method_manager.h (right):

https://codereview.chromium.org/133273032/diff/190001/chromeos/ime/input_meth...
chromeos/ime/input_method_manager.h:121: virtual void EnableLayouts(const
std::string& language_code,
On 2014/01/22 10:01:55, Hiro Komatsu wrote:
> On 2014/01/21 14:05:27, alemate wrote:
> > On 2014/01/21 05:46:01, Hiro Komatsu wrote:
> > > To add keyboard layouts after login, please use EnableInputMethod(s).
> > 
> > What do you mean? This method has always been used for loading locale
layouts
> > after locale switch. It happens only once after user logs in and before
> browser
> > starts.
> > 
> > EnableInputMethod(s) doesn't accept locale.
> 
> InputMethodUtil::GetInputMethodIdsFromLanguageCode converts language_code to
> input_method_id.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> 
> 
> I prefer to use the above function in the caller or add a new function (if
> necessary), rather than a boolean flag of login_layouts_only in this case.

Do you prefer to move EnableLayouts(const std::string& language_code) away from
input manager? Probably I don't understand you:
  There is a moment, when we need to completely replace list of active input
methods by new. We need to calculate this list from different data, including
active locale, active layout and required mode. Or, more generally, from device
settings and user settings.
So, now we have settings selection in chromeos/login code, and input methods
selection (based on these settings) in InputMethodManager::EnableLayouts() code.

  Do you mean, we should eliminate EnableLayouts(), move input methods selection
to chromeos/login code, and use EnableInputMethods from chromeos/login code to
replace enabled methods?

https://codereview.chromium.org/133273032/diff/250001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.h (right):

https://codereview.chromium.org/133273032/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.h:107: bool
IsLoginKeyboard(const std::string& layout) const;
On 2014/01/22 10:01:55, Hiro Komatsu wrote:
> Please add comment and unittest.
> 
> layout -> input_method_id.

Done.

[Hiro Komatsu, 2014-01-24 08:24:44]

https://codereview.chromium.org/133273032/diff/190001/chromeos/ime/input_meth...
File chromeos/ime/input_method_manager.h (right):

https://codereview.chromium.org/133273032/diff/190001/chromeos/ime/input_meth...
chromeos/ime/input_method_manager.h:121: virtual void EnableLayouts(const
std::string& language_code,
On 2014/01/22 11:35:05, alemate wrote:
> On 2014/01/22 10:01:55, Hiro Komatsu wrote:
> > On 2014/01/21 14:05:27, alemate wrote:
> > > On 2014/01/21 05:46:01, Hiro Komatsu wrote:
> > > > To add keyboard layouts after login, please use EnableInputMethod(s).
> > > 
> > > What do you mean? This method has always been used for loading locale
> layouts
> > > after locale switch. It happens only once after user logs in and before
> > browser
> > > starts.
> > > 
> > > EnableInputMethod(s) doesn't accept locale.
> > 
> > InputMethodUtil::GetInputMethodIdsFromLanguageCode converts language_code to
> > input_method_id.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> > 
> > 
> > I prefer to use the above function in the caller or add a new function (if
> > necessary), rather than a boolean flag of login_layouts_only in this case.
> 
> Do you prefer to move EnableLayouts(const std::string& language_code) away
from
> input manager? Probably I don't understand you:
>   There is a moment, when we need to completely replace list of active input
> methods by new. We need to calculate this list from different data, including
> active locale, active layout and required mode. Or, more generally, from
device
> settings and user settings.
> So, now we have settings selection in chromeos/login code, and input methods
> selection (based on these settings) in InputMethodManager::EnableLayouts()
code.

No, what I meant were:

1. Please do not use EnableLayouts to add non login layouts.
1-1. So please do not add the login_layouts_only flag.

2. Please use EnableInputMethod to add non login layouts.
2-1. InputMethodUtil::GetInputMethodIdsFromLanguageCode converts language_code
to input_method_id.
2-2. If necessary, please use both EnableLayouts and EnableInputMethod to switch
the whole input methods.
(2-3. If necessary, please make a new function to perform 2-2 instead of adding
a flag.)


I admit those function names are confusing due to the historical reason.  It
would be a good time to rename them.

 
>   Do you mean, we should eliminate EnableLayouts(), move input methods
selection
> to chromeos/login code, and use EnableInputMethods from chromeos/login code to
> replace enabled methods?

I did not mean so.

[Alexander Alekseev, 2014-01-24 13:29:44]

Please review.

https://codereview.chromium.org/133273032/diff/190001/chromeos/ime/input_meth...
File chromeos/ime/input_method_manager.h (right):

https://codereview.chromium.org/133273032/diff/190001/chromeos/ime/input_meth...
chromeos/ime/input_method_manager.h:121: virtual void EnableLayouts(const
std::string& language_code,
On 2014/01/24 08:24:44, Hiro Komatsu wrote:
> On 2014/01/22 11:35:05, alemate wrote:
> > On 2014/01/22 10:01:55, Hiro Komatsu wrote:
> > > On 2014/01/21 14:05:27, alemate wrote:
> > > > On 2014/01/21 05:46:01, Hiro Komatsu wrote:
> > > > > To add keyboard layouts after login, please use EnableInputMethod(s).
> > > > 
> > > > What do you mean? This method has always been used for loading locale
> > layouts
> > > > after locale switch. It happens only once after user logs in and before
> > > browser
> > > > starts.
> > > > 
> > > > EnableInputMethod(s) doesn't accept locale.
> > > 
> > > InputMethodUtil::GetInputMethodIdsFromLanguageCode converts language_code
to
> > > input_method_id.
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> > > 
> > > 
> > > I prefer to use the above function in the caller or add a new function (if
> > > necessary), rather than a boolean flag of login_layouts_only in this case.
> > 
> > Do you prefer to move EnableLayouts(const std::string& language_code) away
> from
> > input manager? Probably I don't understand you:
> >   There is a moment, when we need to completely replace list of active input
> > methods by new. We need to calculate this list from different data,
including
> > active locale, active layout and required mode. Or, more generally, from
> device
> > settings and user settings.
> > So, now we have settings selection in chromeos/login code, and input methods
> > selection (based on these settings) in InputMethodManager::EnableLayouts()
> code.
> 
> No, what I meant were:
> 
> 1. Please do not use EnableLayouts to add non login layouts.
> 1-1. So please do not add the login_layouts_only flag.
> 
> 2. Please use EnableInputMethod to add non login layouts.
> 2-1. InputMethodUtil::GetInputMethodIdsFromLanguageCode converts language_code
> to input_method_id.
> 2-2. If necessary, please use both EnableLayouts and EnableInputMethod to
switch
> the whole input methods.
> (2-3. If necessary, please make a new function to perform 2-2 instead of
adding
> a flag.)
> 
> 
> I admit those function names are confusing due to the historical reason.  It
> would be a good time to rename them.
> 
>  
> >   Do you mean, we should eliminate EnableLayouts(), move input methods
> selection
> > to chromeos/login code, and use EnableInputMethods from chromeos/login code
to
> > replace enabled methods?
> 
> I did not mean so.
> 

Done.

[Hiro Komatsu, 2014-01-27 05:12:20]

Thanks for the quick update and renaming.

https://codereview.chromium.org/133273032/diff/400001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/133273032/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:151:
InputMethodManagerImpl* manager() const {
If this is no longer necessary, please use manager_.

https://codereview.chromium.org/133273032/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:327:
EXPECT_TRUE(manager()->GetActiveInputMethodIds().front() == "xkb:us::eng");
EXPECT_EQ?

https://codereview.chromium.org/133273032/diff/400001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.h (right):

https://codereview.chromium.org/133273032/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.h:108: bool
IsLoginKeyboard(const std::string& input_method_id) const;
Please keep the same order with .cc file.

[Alexander Alekseev, 2014-01-27 12:50:14]

https://codereview.chromium.org/133273032/diff/400001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/133273032/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:151:
InputMethodManagerImpl* manager() const {
On 2014/01/27 05:12:21, Hiro Komatsu wrote:
> If this is no longer necessary, please use manager_.

Done.

https://codereview.chromium.org/133273032/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:327:
EXPECT_TRUE(manager()->GetActiveInputMethodIds().front() == "xkb:us::eng");
On 2014/01/27 05:12:21, Hiro Komatsu wrote:
> EXPECT_EQ?

Done.

https://codereview.chromium.org/133273032/diff/400001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.h (right):

https://codereview.chromium.org/133273032/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.h:108: bool
IsLoginKeyboard(const std::string& input_method_id) const;
On 2014/01/27 05:12:21, Hiro Komatsu wrote:
> Please keep the same order with .cc file.

Done.

[Nikita (slow), 2014-01-27 19:31:21]

lgtm with nits

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:557: // must match
SwitchLanguageCallback type.
nit:

// SwitchLanguageCallback

Or at least capitalize comment.

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:576: const std::string
login_im =
nit: login_input_method

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:581:
ime_manager->GetInputMethodUtil()->GetLanguageDefaultInputMethodId(
nit: Place GetLanguageDefaultInputMethodId on next line.

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:590: void
SetGuestLocale(UserManager* const usermanager, Profile* const profile) {
nit: const UserManager* user_manager, const Profile* profile

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:659: if
(usermanager->IsUserLoggedIn() && usermanager->IsLoggedInAsGuest()) {
just 

if (UserManager::Get()->IsLoggedInAsGuest())
  SetGuestLocale(...);

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:661: }
nit: drop {}

[Hiro Komatsu, 2014-01-28 06:21:23]

lgtm

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:321:
EXPECT_EQ(manager_->GetActiveInputMethodIds().front(), "xkb:us::eng");
EXPECT_EQ(expected, actual);

[Alexander Alekseev, 2014-01-29 13:36:59]

OK. I'll ship this.

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:557: // must match
SwitchLanguageCallback type.
On 2014/01/27 19:31:21, Nikita Kostylev wrote:
> nit:
> 
> // SwitchLanguageCallback
> 
> Or at least capitalize comment.

Done.

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:576: const std::string
login_im =
On 2014/01/27 19:31:21, Nikita Kostylev wrote:
> nit: login_input_method

Done.

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:581:
ime_manager->GetInputMethodUtil()->GetLanguageDefaultInputMethodId(
On 2014/01/27 19:31:21, Nikita Kostylev wrote:
> nit: Place GetLanguageDefaultInputMethodId on next line.

Done.

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:659: if
(usermanager->IsUserLoggedIn() && usermanager->IsLoggedInAsGuest()) {
On 2014/01/27 19:31:21, Nikita Kostylev wrote:
> just 
> 
> if (UserManager::Get()->IsLoggedInAsGuest())
>   SetGuestLocale(...);

Done.

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:661: }
On 2014/01/27 19:31:21, Nikita Kostylev wrote:
> nit: drop {}

Done.

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/133273032/diff/630001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:321:
EXPECT_EQ(manager_->GetActiveInputMethodIds().front(), "xkb:us::eng");
On 2014/01/28 06:21:24, Hiro Komatsu wrote:
> EXPECT_EQ(expected, actual);

Done.

[commit-bot: I haz the power, 2014-01-29 13:37:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alemate@chromium.org/133273032/660001

[commit-bot: I haz the power, 2014-01-30 01:27:27]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2014-01-30 12:32:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alemate@chromium.org/133273032/660001

[commit-bot: I haz the power, 2014-01-30 13:29:11]

Change committed as 247938