Add support for multiply values in keyboard_layouts to OobeLocalizationTest.

Add support for multiply values in keyboard_layouts to OobeLocalizationTest.

BUG=344390
TEST=manual

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

[Alexander Alekseev, 2014-02-17 20:02:03]

Please review.

[michaelpg, 2014-02-17 22:38:47]

On 2014/02/17 20:02:03, alemate wrote:
> Please review.

LGTM with comments.

[michaelpg, 2014-02-17 22:39:10]

oops.. here are the comments.

https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right):

https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/oobe_localization_browsertest.cc:204: "    throw
'document.querySelector(' + selector + ') failed.';\n"
If this throws, the test will hang until killed. Can we just return the error
string instead of throwing it?

https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/oobe_localization_browsertest.cc:334: "ru,de",
kUSLayout
comma

[dzhioev (left Google), 2014-02-18 01:37:49]

https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right):

https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/oobe_localization_browsertest.cc:147: bool
OobeLocalizationTest::JsExecute(const std::string& expression,
Don't you want to use JSChecker from
chrome/browser/chromeos/login/test/js_checker.h instead?

https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/oobe_localization_browsertest.cc:198: "\n"
I think it's better to define method (named dumpOptionsForTesting, for example)
on JS-side, instead of inlining here. This code is hard to read and modify.

[Alexander Alekseev, 2014-02-18 13:38:43]

https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right):

https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/oobe_localization_browsertest.cc:147: bool
OobeLocalizationTest::JsExecute(const std::string& expression,
On 2014/02/18 01:37:49, dzhioev wrote:
> Don't you want to use JSChecker from
> chrome/browser/chromeos/login/test/js_checker.h instead?

Done.

https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/oobe_localization_browsertest.cc:198: "\n"
On 2014/02/18 01:37:49, dzhioev wrote:
> I think it's better to define method (named dumpOptionsForTesting, for
example)
> on JS-side, instead of inlining here. This code is hard to read and modify.

Discussed offline and decided to leave as it is for now.

https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/oobe_localization_browsertest.cc:204: "    throw
'document.querySelector(' + selector + ') failed.';\n"
On 2014/02/17 22:39:10, Michael Giuffrida wrote:
> If this throws, the test will hang until killed. Can we just return the error
> string instead of throwing it?

Done.

https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/oobe_localization_browsertest.cc:334: "ru,de",
kUSLayout
On 2014/02/17 22:39:10, Michael Giuffrida wrote:
> comma

Done.

[dzhioev (left Google), 2014-02-18 14:13:53]

On 2014/02/18 13:38:43, alemate wrote:
>
https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
> File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right):
> 
>
https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
> chrome/browser/chromeos/login/oobe_localization_browsertest.cc:147: bool
> OobeLocalizationTest::JsExecute(const std::string& expression,
> On 2014/02/18 01:37:49, dzhioev wrote:
> > Don't you want to use JSChecker from
> > chrome/browser/chromeos/login/test/js_checker.h instead?
> 
> Done.
> 
>
https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
> chrome/browser/chromeos/login/oobe_localization_browsertest.cc:198: "\n"
> On 2014/02/18 01:37:49, dzhioev wrote:
> > I think it's better to define method (named dumpOptionsForTesting, for
> example)
> > on JS-side, instead of inlining here. This code is hard to read and modify.
> 
> Discussed offline and decided to leave as it is for now.
> 
>
https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
> chrome/browser/chromeos/login/oobe_localization_browsertest.cc:204: "    throw
> 'document.querySelector(' + selector + ') failed.';\n"
> On 2014/02/17 22:39:10, Michael Giuffrida wrote:
> > If this throws, the test will hang until killed. Can we just return the
error
> > string instead of throwing it?
> 
> Done.
> 
>
https://codereview.chromium.org/167013004/diff/1/chrome/browser/chromeos/logi...
> chrome/browser/chromeos/login/oobe_localization_browsertest.cc:334: "ru,de",
> kUSLayout
> On 2014/02/17 22:39:10, Michael Giuffrida wrote:
> > comma
> 
> Done.
LGTM

[Alexander Alekseev, 2014-02-18 14:16:22]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-02-18 14:16:30]

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

[commit-bot: I haz the power, 2014-02-18 16:41:58]

Change committed as 251780