We should switch the keyboard layout to the layout the user set according to

We should switch the keyboard layout to the layout the user set according to which user POD is selected.

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

[Nikita (slow), 2013-07-09 13:21:57]

You'll need someone from
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/ime/OWNER...

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

[Nikita (slow), 2013-07-09 13:24:31]

Where latin-based keyboard layout's are filtered?
CL summary should mention that.

[Alexander Alekseev, 2013-07-10 16:21:58]

Added check for "Full Latin Keyboard language".

I created table of such languages manually googling, reading Wikipedia, and
testing a few layouts by hand.

Among current languages in chromeos/ime/input_methods.txt there seems quite a
few not having this feature:

"be",     // Belarusian
"bg",     // Bulgarian
"el",     // Greek
"he",     // Hebrew
"hy",     // Armenian
"ja",     // Japanese
"ka",     // Georgian
"mn",     // Mongolian
"ru",     // Russian
"sr",     // Serbian
"uk"      // Ukrainian

All the others seem to have all 26 Latin letters on keyboard, despite not having
some of them in alphabet.

The list of "Full latin Keyboard Languages" is in
kFullLatinKeyboardLanguageList[] in
chrome/browser/chromeos/input_method/input_method_manager_impl.cc . 

I think it should be somehow verified.

A special note to nona:
Also for the check to be fast, I use sorted vector of two-letter language codes
(as vector<uint_16>) and a special type
InputMethodManagerImpl::TwoLetterLanguageCode (so that actually
vector<TwoLetterLanguageCode> is stored). Do you think it's ok to modify
input_method_manager_impl.{cc,h} this way, or should I put this code to some
other file?

[Nikita (slow), 2013-07-11 08:54:55]

Please fix unit_test:
[ RUN      ] InputMethodPersistenceTest.TestPrefPersistenceByState
[29587:29587:0711/013832:2507777282366:FATAL:input_method_persistence.cc(44)]
Check failed: manager. 
[29587:29587:0711/013832:2507777282366:FATAL:input_method_persistence.cc(44)]
Check failed: manager. 
Received signal 6
 [0x7f9d9ac3d58e] base::debug::StackTrace::StackTrace()
 [0x7f9d9ac3dab8] base::debug::(anonymous namespace)::StackDumpSignalHandler()
 [0x7f9d947d6cb0] <unknown>
 [0x7f9d93519425] gsignal
 [0x7f9d9351cb8b] abort
 [0x7f9d9ac3c4c9] base::debug::BreakDebugger()
 [0x7f9d9ac64c64] logging::LogMessage::~LogMessage()
 [0x000003547e84]
_ZN8chromeos12input_method12_GLOBAL__N_1L21SetUserLRUInputMethodERKSs.constprop.25
 [0x0000035486b3]
chromeos::input_method::InputMethodPersistence::InputMethodChanged()
 [0x0000009c8a93]
chromeos::input_method::InputMethodPersistenceTest_TestPrefPersistenceByState_Test::TestBody()
 [0x00000373a99a] testing::Test::Run()
 [0x00000373aa5a] testing::TestInfo::Run()
 [0x00000373ab9f] testing::TestCase::Run()
 [0x00000373ae2f] testing::internal::UnitTestImpl::RunAllTests()
 [0x000003735823] testing::UnitTest::Run()
 [0x000003f135de] base::TestSuite::Run()
 [0x000003d4c33a] main
 [0x7f9d9350476d] __libc_start_main
 [0x00000050c499] <unknown>

[Nikita (slow), 2013-07-11 08:59:56]

Please review:

nona@ for
chrome/browser/chromeos/input_method/*
chromeos/ime/*

mnissler@ for chrome/browser/prefs.*

I'll review chrome/browser/ui/webui/chromeos/login/*

[Nikita (slow), 2013-07-11 09:13:53]

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

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:985: #if
defined(OS_CHROMEOS)
No need for OS_CHROMEOS here as this class is already located in chromeos
specific folder.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:994: #if
defined(OS_CHROMEOS)
nit: same here.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:996: if
(g_browser_process == NULL)
nit: drop this check.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:999: if
(!base::chromeos::IsRunningOnChromeOS())
What about relying on stubs instead when not running on a real Chrome OS VM
image/device?

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1003:
DCHECK(local_state);
nit: Drop this DCHECK.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1005: if
(local_state == NULL)
nit: Drop this.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1010:
DCHECK(manager);
nit: Drop this DCHECK.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1013: do {
You should delete this loop it is not really needed.
My proposal:

Have a utility method that tries to switch to user specific input method and
returns bool. Code that calls it will switch to default if needed.

Introducing this method will also help split this method into smaller parts.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1018:
DLOG(WARNING) << "SetUserInputMethod('" << username
nit: I guess it is perfectly normal situation till at least one user signs in
that has latin-based input_methods.

No need for DLOG(WARNING) here.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1027:
DLOG(WARNING) << "SetUserInputMethod('" << username
nit: Same here. This looks like a perfectly normal (rare but normal) situation
so no need for DLOG(WARNING).

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1032: if
(manager == NULL)
You don't have to check for manager not being NULL. It is assumed it is always
not NULL when we are accessing it. You could check its lifetime if you want.

If this assumption is not true this code better crash and generate crash report
instead of doing nothing.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1037:
DCHECK(ime_util);
nit: Drop DCHECK

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1039: if
(ime_util == NULL)
Same here.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1047:
LOG(WARNING) << "SetUserInputMethod('" << username
nit: I propose dropping log message as well and just doing pref cleanup instead.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1056:
->GetActiveInputMethodIds(),
nit: -> should be on the previous line

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1058: if
(!chromeos::input_method::InputMethodManager::Get()->EnableInputMethod(
nit:

if (!chromeos::input_method::InputMethodManager::Get()->
        EnableInputMethod(input_method)) {

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1065:
chromeos::input_method::InputMethodManager::Get()->ChangeInputMethod(
nit: this looks more readable:

chromeos::input_method::InputMethodManager::Get()->
    ChangeInputMethod(input_method);

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1075:
DLOG(WARNING)
nit: No need for DLOG(WARNING), perfectly normal situation.

[Seigo Nonaka, 2013-07-11 09:43:55]

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:125: const
char* kFullLatinKeyboardLanguageList[] = {
Please notice that there is "xkb:jp::jpn" keyboard which is for Latin character
input in jp109 keyboard. (http://en.wikipedia.org/wiki/Keyboard_layout#Japanese)

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right):

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:102: virtual
bool IsLanguageFullLatinKeyboard(const std::string& lang) const
As I mentioned in crbug.com/152843, please consider to use IsKeyboardLayout
function instead.

If you feel bad for non-Latin keyboard on login screen like Russian keyboard,
please do as separate work. (I recommend getting advise from UX or i18n team's).

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:191: // Start
or stop the system input method framework as needed.
nit: Starts or stops

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/input_method_persistence.cc (right):

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:32: #if
defined(OS_CHROMEOS)
nit: no need to add OS_CHROMEOS. This class is only used in chromeos
environment.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:37: if
(local_state != NULL) {
If you put DCHECK, please remove this condition, if local_state can be NULL,
please remove DCHECK.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:46: if (manager
== NULL)
no need to check manager, you already put DCHECK above.
I think this shouldn't be NULL, so just DCHECK is fine :)

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:53: if
(ime_util == NULL)
ditto.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:57:
ime_util->GetLanguageCodeFromInputMethodId(input_method)))
Please note that third party input can set any language, so we should check
IsKeyboardLayout.

[Mattias Nissler (ping if slow), 2013-07-11 10:05:48]

LGTM once you get coding style sorted (other reviewers cover that) and fix pref
name declaration.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:335: // starts
or stops the system input method framework as needed.
nit: Starts

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:340: if
(ContainOnlyKeyboardLayout(active_input_method_ids_)) {
nit: ContainOnlyKeyboardLayout should be ContainsOnlyKeyboardLayout (imperative
"contain" certainly doesn't make sense here).

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:342: // at
crosbug.com/27051.
nit: new code should refer to crbug.com IMHO

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/input_method_persistence.cc (right):

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:35:
DCHECK(local_state);
Not needed, local_state != NULL should be guaranteed by initialization order. If
it's not, you have bigger problems :)

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:44:
DCHECK(manager);
Does this singleton getter actually return NULL in any circumstance? Being
overly defensive makes code unreadable.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:65: // updater
may have side-effects, therefore we do not replace
*Updater

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

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1108: // Set
Least Recently used input method for the user
nit: missing period.

https://codereview.chromium.org/18856014/diff/29001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/18856014/diff/29001/chrome/common/pref_names....
chrome/common/pref_names.cc:2508: // (used at login screen)
nit: Missing period, not using the full line width on line 2507 for the comment.

https://codereview.chromium.org/18856014/diff/29001/chrome/common/pref_names....
chrome/common/pref_names.cc:2509: extern const char kUsersLRUInputMethod[] =
"UsersLRUInputMethod";
This should be earlier in this file (the file as three sections "PROFILE PREFS",
"LOCAL STATE", "SERVICE PREFS") in the "LOCAL STATE" section, note that there's
a whole section of Chrome OS section there where you can add your pref. I know
I'm fighting a lost battle here, but still :)

https://codereview.chromium.org/18856014/diff/29001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/18856014/diff/29001/chrome/common/pref_names....
chrome/common/pref_names.h:940: extern const char kUsersLRUInputMethod[];
When you change definition order in pref_names.cc, please adjust this file to be
consistent.

[Alexander Alekseev, 2013-07-12 20:10:48]

- Added checking against IsKeyboardLayout().
- Added support for languages with some Full Latin Keyboard layouts (like
"xkb:jp::jpn").
- Moved checker code to separate files.
- (probably) fixed all style inconsistencies.

Please review.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:125: const
char* kFullLatinKeyboardLanguageList[] = {
On 2013/07/11 09:43:56, Seigo Nonaka wrote:
> Please notice that there is "xkb:jp::jpn" keyboard which is for Latin
character
> input in jp109 keyboard.
(http://en.wikipedia.org/wiki/Keyboard_layout#Japanese)
> 

That's a good point. I've been waiting for something like that. ;-)

I think I should extend this structure to keep a list of "explicitly allowed
keyboards for this language".
NULL would mean "any". So that I can have only one list for "jp" with a single
entry "xkb:jp::jpn" for now.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:335: // starts
or stops the system input method framework as needed.
On 2013/07/11 10:05:49, Mattias Nissler wrote:
> nit: Starts

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:340: if
(ContainOnlyKeyboardLayout(active_input_method_ids_)) {
On 2013/07/11 10:05:49, Mattias Nissler wrote:
> nit: ContainOnlyKeyboardLayout should be ContainsOnlyKeyboardLayout
(imperative
> "contain" certainly doesn't make sense here).

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:342: // at
crosbug.com/27051.
On 2013/07/11 10:05:49, Mattias Nissler wrote:
> nit: new code should refer to http://crbug.com IMHO

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right):

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:102: virtual
bool IsLanguageFullLatinKeyboard(const std::string& lang) const
On 2013/07/11 09:43:56, Seigo Nonaka wrote:
> As I mentioned in crbug.com/152843, please consider to use IsKeyboardLayout
> function instead.
> 
> If you feel bad for non-Latin keyboard on login screen like Russian keyboard,
> please do as separate work. (I recommend getting advise from UX or i18n
team's).
> 

It has already been discussed a bit in
https://code.google.com/p/chromium/issues/detail?id=152843 . I think I'll add
people from UX & i18n there.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:191: // Start
or stop the system input method framework as needed.
On 2013/07/11 09:43:56, Seigo Nonaka wrote:
> nit: Starts or stops

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/input_method_persistence.cc (right):

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:32: #if
defined(OS_CHROMEOS)
On 2013/07/11 09:43:56, Seigo Nonaka wrote:
> nit: no need to add OS_CHROMEOS. This class is only used in chromeos
> environment.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:35:
DCHECK(local_state);
On 2013/07/11 10:05:49, Mattias Nissler wrote:
> Not needed, local_state != NULL should be guaranteed by initialization order.
If
> it's not, you have bigger problems :)

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:37: if
(local_state != NULL) {
On 2013/07/11 09:43:56, Seigo Nonaka wrote:
> If you put DCHECK, please remove this condition, if local_state can be NULL,
> please remove DCHECK.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:44:
DCHECK(manager);
On 2013/07/11 10:05:49, Mattias Nissler wrote:
> Does this singleton getter actually return NULL in any circumstance? Being
> overly defensive makes code unreadable.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:46: if (manager
== NULL)
On 2013/07/11 09:43:56, Seigo Nonaka wrote:
> no need to check manager, you already put DCHECK above.
> I think this shouldn't be NULL, so just DCHECK is fine :)

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:53: if
(ime_util == NULL)
On 2013/07/11 09:43:56, Seigo Nonaka wrote:
> ditto.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:57:
ime_util->GetLanguageCodeFromInputMethodId(input_method)))
On 2013/07/11 09:43:56, Seigo Nonaka wrote:
> Please note that third party input can set any language, so we should check
> IsKeyboardLayout. 

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:65: // updater
may have side-effects, therefore we do not replace
On 2013/07/11 10:05:49, Mattias Nissler wrote:
> *Updater

Done.

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

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:985: #if
defined(OS_CHROMEOS)
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> No need for OS_CHROMEOS here as this class is already located in chromeos
> specific folder.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:994: #if
defined(OS_CHROMEOS)
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> nit: same here.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:996: if
(g_browser_process == NULL)
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> nit: drop this check.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:999: if
(!base::chromeos::IsRunningOnChromeOS())
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> What about relying on stubs instead when not running on a real Chrome OS VM
> image/device?

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1003:
DCHECK(local_state);
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> nit: Drop this DCHECK.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1005: if
(local_state == NULL)
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> nit: Drop this.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1010:
DCHECK(manager);
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> nit: Drop this DCHECK.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1013: do {
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> You should delete this loop it is not really needed.
> My proposal:
> 
> Have a utility method that tries to switch to user specific input method and
> returns bool. Code that calls it will switch to default if needed.
> 
> Introducing this method will also help split this method into smaller parts.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1018:
DLOG(WARNING) << "SetUserInputMethod('" << username
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> nit: I guess it is perfectly normal situation till at least one user signs in
> that has latin-based input_methods.
> 
> No need for DLOG(WARNING) here.

Do all our users always have some latin input method enabled ?
Probably HW-default is always enabled, I think.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1027:
DLOG(WARNING) << "SetUserInputMethod('" << username
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> nit: Same here. This looks like a perfectly normal (rare but normal) situation
> so no need for DLOG(WARNING).

But it's debug log, isn't it? I think I'd better have a trace of rare events at
least in debug log.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1032: if
(manager == NULL)
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> You don't have to check for manager not being NULL. It is assumed it is always
> not NULL when we are accessing it. You could check its lifetime if you want.
> 
> If this assumption is not true this code better crash and generate crash
report
> instead of doing nothing.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1037:
DCHECK(ime_util);
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> nit: Drop DCHECK

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1039: if
(ime_util == NULL)
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> Same here.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1047:
LOG(WARNING) << "SetUserInputMethod('" << username
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> nit: I propose dropping log message as well and just doing pref cleanup
instead.

It is probably dangerous to silently drop user preferences. I've added pref
cleanup, but left log message. Do you think it is OK now?

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1056:
->GetActiveInputMethodIds(),
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> nit: -> should be on the previous line

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1058: if
(!chromeos::input_method::InputMethodManager::Get()->EnableInputMethod(
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> nit:
> 
> if (!chromeos::input_method::InputMethodManager::Get()->
>         EnableInputMethod(input_method)) {

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1065:
chromeos::input_method::InputMethodManager::Get()->ChangeInputMethod(
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> nit: this looks more readable:
> 
> chromeos::input_method::InputMethodManager::Get()->
>     ChangeInputMethod(input_method);

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1075:
DLOG(WARNING)
On 2013/07/11 09:13:54, Nikita Kostylev wrote:
> nit: No need for DLOG(WARNING), perfectly normal situation.

But it's debug log. Let's leave traces of rare events at least in debug log.

https://codereview.chromium.org/18856014/diff/29001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1108: // Set
Least Recently used input method for the user
On 2013/07/11 10:05:49, Mattias Nissler wrote:
> nit: missing period.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/18856014/diff/29001/chrome/common/pref_names....
chrome/common/pref_names.cc:2508: // (used at login screen)
On 2013/07/11 10:05:49, Mattias Nissler wrote:
> nit: Missing period, not using the full line width on line 2507 for the
comment.

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/common/pref_names....
chrome/common/pref_names.cc:2509: extern const char kUsersLRUInputMethod[] =
"UsersLRUInputMethod";
On 2013/07/11 10:05:49, Mattias Nissler wrote:
> This should be earlier in this file (the file as three sections "PROFILE
PREFS",
> "LOCAL STATE", "SERVICE PREFS") in the "LOCAL STATE" section, note that
there's
> a whole section of Chrome OS section there where you can add your pref. I know
> I'm fighting a lost battle here, but still :)

Done.

https://codereview.chromium.org/18856014/diff/29001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/18856014/diff/29001/chrome/common/pref_names....
chrome/common/pref_names.h:940: extern const char kUsersLRUInputMethod[];
On 2013/07/11 10:05:49, Mattias Nissler wrote:
> When you change definition order in pref_names.cc, please adjust this file to
be
> consistent.

Done.

[Nikita (slow), 2013-07-15 12:56:37]

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

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:110: static bool
Contains(const std::vector<std::string>& container,
nit: no need to mark function static. Usually such utility functions are just
declared in unnamed namespace

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:992: static bool
SetUserInputMethodImpl(
Please move this helper method to unnamed namespace in this file and drop
static.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Nonmember,_Sta...

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

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:196: // Required
Local State preferences
nit: dot is missing.

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:385: // Update
current input method (namely keyboard layout) to LRU by this user
nit: dot is missing.

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:388: // Update
current input method to HW default
nit: dot is missing.

[Alexander Alekseev, 2013-07-17 16:11:51]

Mattias Nissler, Seigo Nonaka

I've updated code. Please review again. Thank you.




On 2013/07/11 09:43:55, Seigo Nonaka wrote:
>
https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
> File chrome/browser/chromeos/input_method/input_method_manager_impl.cc
(right):
> 
>
https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
> chrome/browser/chromeos/input_method/input_method_manager_impl.cc:125: const
> char* kFullLatinKeyboardLanguageList[] = {
> Please notice that there is "xkb:jp::jpn" keyboard which is for Latin
character
> input in jp109 keyboard.
(http://en.wikipedia.org/wiki/Keyboard_layout#Japanese)
> 
>
https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
> File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right):
> 
>
https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
> chrome/browser/chromeos/input_method/input_method_manager_impl.h:102: virtual
> bool IsLanguageFullLatinKeyboard(const std::string& lang) const
> As I mentioned in crbug.com/152843, please consider to use IsKeyboardLayout
> function instead.
> 
> If you feel bad for non-Latin keyboard on login screen like Russian keyboard,
> please do as separate work. (I recommend getting advise from UX or i18n
team's).
> 
>
https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
> chrome/browser/chromeos/input_method/input_method_manager_impl.h:191: // Start
> or stop the system input method framework as needed.
> nit: Starts or stops
> 
>
https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
> File chrome/browser/chromeos/input_method/input_method_persistence.cc (right):
> 
>
https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
> chrome/browser/chromeos/input_method/input_method_persistence.cc:32: #if
> defined(OS_CHROMEOS)
> nit: no need to add OS_CHROMEOS. This class is only used in chromeos
> environment.
> 
>
https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
> chrome/browser/chromeos/input_method/input_method_persistence.cc:37: if
> (local_state != NULL) {
> If you put DCHECK, please remove this condition, if local_state can be NULL,
> please remove DCHECK.
> 
>
https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
> chrome/browser/chromeos/input_method/input_method_persistence.cc:46: if
(manager
> == NULL)
> no need to check manager, you already put DCHECK above.
> I think this shouldn't be NULL, so just DCHECK is fine :)
> 
>
https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
> chrome/browser/chromeos/input_method/input_method_persistence.cc:53: if
> (ime_util == NULL)
> ditto.
> 
>
https://codereview.chromium.org/18856014/diff/29001/chrome/browser/chromeos/i...
> chrome/browser/chromeos/input_method/input_method_persistence.cc:57:
> ime_util->GetLanguageCodeFromInputMethodId(input_method)))
> Please note that third party input can set any language, so we should check
> IsKeyboardLayout.

[Seigo Nonaka, 2013-07-22 13:17:34]

lgtm but please wait Nikita's review :)

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/input_method_persistence.cc (right):

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:79: <<
prefs::kUsersLRUInputMethod << "' for '" << username
nit: indent?

https://codereview.chromium.org/18856014/diff/67001/chromeos/ime/input_method...
File chromeos/ime/input_methods.txt (right):

https://codereview.chromium.org/18856014/diff/67001/chromeos/ime/input_method...
chromeos/ime/input_methods.txt:64: #  
chrome/browser/chromeos/input_method/input_method_manager_impl_ll.cc
No need to do now, but how about introducing new low into following entries to
be able to know the target keyboard layout is fully alphabetic keyboard or not?

[Seigo Nonaka, 2013-07-22 14:14:40]

https://codereview.chromium.org/18856014/diff/67001/chromeos/ime/input_method...
File chromeos/ime/input_methods.txt (right):

https://codereview.chromium.org/18856014/diff/67001/chromeos/ime/input_method...
chromeos/ime/input_methods.txt:64: #  
chrome/browser/chromeos/input_method/input_method_manager_impl_ll.cc
On 2013/07/22 13:17:34, Seigo Nonaka wrote:
> No need to do now, but how about introducing new low into following entries to
> be able to know the target keyboard layout is fully alphabetic keyboard or
not?

Ugh, /low/column/s please...

[Alexander Alekseev, 2013-07-22 14:24:20]

I cleaned code, probably style is correct now.

Please review.

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/input_method_persistence.cc (right):

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:79: <<
prefs::kUsersLRUInputMethod << "' for '" << username
On 2013/07/22 13:17:34, Seigo Nonaka wrote:
> nit: indent?

Done.

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

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:110: static bool
Contains(const std::vector<std::string>& container,
On 2013/07/15 12:56:38, Nikita Kostylev wrote:
> nit: no need to mark function static. Usually such utility functions are just
> declared in unnamed namespace

Both gcc/clang do not warn on unused non-static functions in unnamed namespaces.
Don't know why. So let's use static inside unnamed namespaces.

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:992: static bool
SetUserInputMethodImpl(
On 2013/07/15 12:56:38, Nikita Kostylev wrote:
> Please move this helper method to unnamed namespace in this file and drop
> static.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Nonmember%2C_S...

Moved to unnamed namespace and left it static, because compilers do not warn on
unused non-static functions in unnamed namespaces.

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

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:196: // Required
Local State preferences
On 2013/07/15 12:56:38, Nikita Kostylev wrote:
> nit: dot is missing.

Done.

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:385: // Update
current input method (namely keyboard layout) to LRU by this user
On 2013/07/15 12:56:38, Nikita Kostylev wrote:
> nit: dot is missing.

Done.

https://codereview.chromium.org/18856014/diff/67001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:388: // Update
current input method to HW default
On 2013/07/15 12:56:38, Nikita Kostylev wrote:
> nit: dot is missing.

Done.

https://codereview.chromium.org/18856014/diff/67001/chromeos/ime/input_method...
File chromeos/ime/input_methods.txt (right):

https://codereview.chromium.org/18856014/diff/67001/chromeos/ime/input_method...
chromeos/ime/input_methods.txt:64: #  
chrome/browser/chromeos/input_method/input_method_manager_impl_ll.cc
On 2013/07/22 13:17:34, Seigo Nonaka wrote:
> No need to do now, but how about introducing new low into following entries to
> be able to know the target keyboard layout is fully alphabetic keyboard or
not?

Probably, but do we have any plugin-based layouts not mentioned here? Also the
first comment in this file suggests not to add unnecessary layouts here. 

I was thinking of API to directly check a letter through already loaded keyboard
layout. But it looks like a lot of new code all over the system... So I hope
someone would once get a strong reason to create it ;-)

[Nikita (slow), 2013-07-22 18:50:31]

lgtm

I've executed few trybots.
Once they are green please check commit queue checkbox.

Context: just checking CQ checkbox may not be sufficient as some trybots are not
included (like cros_*) and other trybots do run limited number of tests.

[commit-bot: I haz the power, 2013-07-23 12:46:08]

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

[commit-bot: I haz the power, 2013-07-23 14:12:14]

Retried try job too often on mac_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-07-23 14:42:46]

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

[commit-bot: I haz the power, 2013-07-23 17:11:28]

Change committed as 213144