ChromeOS: "Add New User" screen should enable all hardware keyboards.

ChromeOS: "Add New User" screen should enable all hardware keyboards.

"Add New User" screen should have enabled all hardware
keyboards, owner and system input methods.

BUG=365299
TEST=manual

Committed: https://crrev.com/a5a81ef618a26e513a4ae0f2b4b0e77f8224e946
Cr-Commit-Position: refs/heads/master@{#296199}

Committed: https://crrev.com/dee7902a7ee2b99d27dd54b91b8f266427490c96
Cr-Commit-Position: refs/heads/master@{#296408}

[Alexander Alekseev, 2014-08-23 03:42:01]

Please review.

[Alexander Alekseev, 2014-08-23 03:42:02]

Please review.

[dzhioev (left Google), 2014-08-25 18:00:35]

On 2014/08/23 03:42:02, alemate wrote:
> Please review.

Please implement tests.

[Nikita (slow), 2014-09-16 20:53:41]

On 2014/08/25 18:00:35, dzhioev wrote:
> On 2014/08/23 03:42:02, alemate wrote:
> > Please review.
> 
> Please implement tests.

Take a look at tests that use LoginManagerTest base class.

[Alexander Alekseev, 2014-09-19 18:39:25]

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

[Alexander Alekseev, 2014-09-19 18:39:26]

Please review.

+shuchen for input_method_persistence.*

[dzhioev (left Google), 2014-09-22 11:54:59]

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc (right):

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
Use $('identifier') instead of document.querySelector('#identifier')

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
Do we have a test, where we move focus from one pod to another?

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:34: void
ReportInputMethodsDifference(const std::string& where,
I don't think this function is needed. Just overload operator<<(ostream&,
std::vector<std::string>), and you will be able to use EXPECT_EQ with a vectors.
After that all invocations of this function could be replaced with
EXPECT_EQ(expected, input_method::InputMethodManager::Get()
                        ->GetActiveIMEState()
                        ->GetActiveInputMethodIds());

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:102:
JSExpect("!!document.querySelector('#pod-row')");
Please, remove these two lines from all test cases. They are not related to the
feature you are testing.

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:104:
"document.querySelectorAll('.pod:not(#user-pod-template)').length == 2");
"$('pod-row').pods.length == 2"

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:207:
expected_input_methods.push_back("xkb:us:colemak:eng");
You repeat these 5 lines many times in this file. Please create a method, that
appends these methods to the list (something like 'void
AppendLocaleInputMethods(vector<string>& methods)').

[Shu Chen, 2014-09-22 14:03:33]

lgtm for changes under 	chrome/browser/chromeos/input_method.

[Alexander Alekseev, 2014-09-22 19:44:58]

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc (right):

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2014/09/22 11:54:59, dzhioev wrote:
> Use $('identifier') instead of document.querySelector('#identifier')

Done.

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2014/09/22 11:54:59, dzhioev wrote:
> Do we have a test, where we move focus from one pod to another?

Done.

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:34: void
ReportInputMethodsDifference(const std::string& where,
On 2014/09/22 11:54:59, dzhioev wrote:
> I don't think this function is needed. Just overload operator<<(ostream&,
> std::vector<std::string>), and you will be able to use EXPECT_EQ with a
vectors.
> After that all invocations of this function could be replaced with
> EXPECT_EQ(expected, input_method::InputMethodManager::Get()
>                         ->GetActiveIMEState()
>                         ->GetActiveInputMethodIds());

Done.

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:102:
JSExpect("!!document.querySelector('#pod-row')");
On 2014/09/22 11:54:59, dzhioev wrote:
> Please, remove these two lines from all test cases. They are not related to
the
> feature you are testing.

Done.

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:104:
"document.querySelectorAll('.pod:not(#user-pod-template)').length == 2");
On 2014/09/22 11:54:59, dzhioev wrote:
> "$('pod-row').pods.length == 2"

Done.

https://codereview.chromium.org/484353005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:207:
expected_input_methods.push_back("xkb:us:colemak:eng");
On 2014/09/22 11:54:59, dzhioev wrote:
> You repeat these 5 lines many times in this file. Please create a method, that
> appends these methods to the list (something like 'void
> AppendLocaleInputMethods(vector<string>& methods)').

Done.

[dzhioev (left Google), 2014-09-23 11:05:39]

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc (right):

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:24: #include
"content/public/browser/notification_types.h"
Why do we need all this notifications_*.h included? I can't find where they are
used.

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:36: class
ENLocale {
nit: Why do we need a whole class for that? I think a function will be enough.
And ENLocale is not a good name. I don't understand what it means.

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:44:
chromeos::input_method::InputMethodManager::Get()->MigrateInputMethods(
Do we need to do a migration here? We are doing it in tests cases anyway.

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:120:
en_locale.reset(new ENLocale);
Why not in constructor?

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:137:
std::vector<std::string> user_input_methods;
Is this a mapping from user index to user's input method?

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:198:
->GetActiveInputMethodIds());
These lines (190-198) are repeated 4 times in this file. Could you please create
a method in the base class doing these things.

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:211: class
LoginUIKeyboardTestWithUsersAndOwner : public chromeos::LoginManagerTest {
LoginUIKeyboardTestWithUsersAndOwner has a common parts with LoginUIKeyboardTest
(user_input_methods, en_locale). Could you please create a common base for them.

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:273:
input_method::InputMethodManager::Get()->GetActiveIMEState();
|ime_state| not used.

[Alexander Alekseev, 2014-09-23 13:31:40]

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc (right):

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:24: #include
"content/public/browser/notification_types.h"
On 2014/09/23 11:05:39, dzhioev wrote:
> Why do we need all this notifications_*.h included? I can't find where they
are
> used.

Done.

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:36: class
ENLocale {
On 2014/09/23 11:05:39, dzhioev wrote:
> nit: Why do we need a whole class for that? I think a function will be enough.
> And ENLocale is not a good name. I don't understand what it means.

Done.

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:44:
chromeos::input_method::InputMethodManager::Get()->MigrateInputMethods(
On 2014/09/23 11:05:38, dzhioev wrote:
> Do we need to do a migration here? We are doing it in tests cases anyway.

I've removed Migration from tests.

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:120:
en_locale.reset(new ENLocale);
On 2014/09/23 11:05:38, dzhioev wrote:
> Why not in constructor?

Because of MigrateInputMethods.

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:137:
std::vector<std::string> user_input_methods;
On 2014/09/23 11:05:39, dzhioev wrote:
> Is this a mapping from user index to user's input method? 

Yes.

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:198:
->GetActiveInputMethodIds());
On 2014/09/23 11:05:38, dzhioev wrote:
> These lines (190-198) are repeated 4 times in this file. Could you please
create
> a method in the base class doing these things.

I've removed most of them.

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:211: class
LoginUIKeyboardTestWithUsersAndOwner : public chromeos::LoginManagerTest {
On 2014/09/23 11:05:39, dzhioev wrote:
> LoginUIKeyboardTestWithUsersAndOwner has a common parts with
LoginUIKeyboardTest
> (user_input_methods, en_locale). Could you please create a common base for
them.

user_input_methods are different. That is why classes are different.

https://codereview.chromium.org/484353005/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc:273:
input_method::InputMethodManager::Get()->GetActiveIMEState();
On 2014/09/23 11:05:39, dzhioev wrote:
> |ime_state| not used.

Done.

[dzhioev (left Google), 2014-09-23 14:32:18]

LGTM with nits.

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

https://codereview.chromium.org/484353005/diff/220001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:255: // Returns
Least Used User login Input method.
Fix capital letters.

https://codereview.chromium.org/484353005/diff/220001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/484353005/diff/220001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:990:
'browser/chromeos/login/login_ui_keyboard_browsertest.cc',
Keep alphabetical order.

[Alexander Alekseev, 2014-09-23 14:39:04]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2014-09-23 14:39:37]

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

https://codereview.chromium.org/484353005/diff/220001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:255: // Returns
Least Used User login Input method.
On 2014/09/23 14:32:18, dzhioev wrote:
> Fix capital letters.

Done.

https://codereview.chromium.org/484353005/diff/220001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/484353005/diff/220001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:990:
'browser/chromeos/login/login_ui_keyboard_browsertest.cc',
On 2014/09/23 14:32:18, dzhioev wrote:
> Keep alphabetical order.

Done.

[commit-bot: I haz the power, 2014-09-23 14:40:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/484353005/240001

[commit-bot: I haz the power, 2014-09-23 14:43:20]

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

[commit-bot: I haz the power, 2014-09-23 14:43:22]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/59480)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/64359)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Alexander Alekseev, 2014-09-23 15:39:16]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-09-23 15:40:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/484353005/260001

[commit-bot: I haz the power, 2014-09-23 16:32:49]

Committed patchset #14 (id:260001) as 1c8af00501e80c12b8fed9668abad69ca55ff82c

[commit-bot: I haz the power, 2014-09-23 16:33:51]

Patchset 14 (id:??) landed as
https://crrev.com/a5a81ef618a26e513a4ae0f2b4b0e77f8224e946
Cr-Commit-Position: refs/heads/master@{#296199}

[Nico, 2014-09-23 22:46:01]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/597723003/ by thakis@chromium.org.

The reason for reverting is: Speculative, looks like this broke a bunch of
browser_tests in dbg:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....

[Alexander Alekseev, 2014-09-24 13:50:10]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-09-24 13:51:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/484353005/280001

[commit-bot: I haz the power, 2014-09-24 14:31:45]

Committed patchset #15 (id:280001) as 61cdf9cffef13cd226dd59e1a7bdbac082bbccfc

[commit-bot: I haz the power, 2014-09-24 14:32:18]

Patchset 15 (id:??) landed as
https://crrev.com/dee7902a7ee2b99d27dd54b91b8f266427490c96
Cr-Commit-Position: refs/heads/master@{#296408}