Add a full QWERTY layout.

Adds a full QWERTY layout to the Virtual Keyboard.


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

[bshe, 2013-10-08 14:58:06]

https://codereview.chromium.org/26258003/diff/5001/ui/keyboard/keyboard_util.cc
File ui/keyboard/keyboard_util.cc (right):

https://codereview.chromium.org/26258003/diff/5001/ui/keyboard/keyboard_util....
ui/keyboard/keyboard_util.cc:187: {"keyboard/layouts/system.html",
IDR_KEYBOARD_LAYOUTS_SYSTEM},
nit: alphabetical please

https://codereview.chromium.org/26258003/diff/5001/ui/keyboard/resources/elem...
File ui/keyboard/resources/elements/kb-key-sequence.html (right):

https://codereview.chromium.org/26258003/diff/5001/ui/keyboard/resources/elem...
ui/keyboard/resources/elements/kb-key-sequence.html:50: if(this.invert) {
Is the keycode still right after this key inverted?

[kevers, 2013-10-08 15:39:48]

Drive by review.

https://codereview.chromium.org/26258003/diff/5001/ui/keyboard/keyboard_util.cc
File ui/keyboard/keyboard_util.cc (right):

https://codereview.chromium.org/26258003/diff/5001/ui/keyboard/keyboard_util....
ui/keyboard/keyboard_util.cc:187: {"keyboard/layouts/system.html",
IDR_KEYBOARD_LAYOUTS_SYSTEM},
On 2013/10/08 14:58:06, bshe wrote:
> nit: alphabetical please

Also suggest using the name "system-qwerty.html" instead of system as there will
likely be several variants of the system keyboard based on locale.

[rsadam, 2013-10-08 21:36:14]

https://chromiumcodereview.appspot.com/26258003/diff/5001/ui/keyboard/keyboar...
File ui/keyboard/keyboard_util.cc (right):

https://chromiumcodereview.appspot.com/26258003/diff/5001/ui/keyboard/keyboar...
ui/keyboard/keyboard_util.cc:187: {"keyboard/layouts/system.html",
IDR_KEYBOARD_LAYOUTS_SYSTEM},
On 2013/10/08 15:39:48, kevers wrote:
> On 2013/10/08 14:58:06, bshe wrote:
> > nit: alphabetical please
> 
> Also suggest using the name "system-qwerty.html" instead of system as there
will
> likely be several variants of the system keyboard based on locale.

Done.

https://chromiumcodereview.appspot.com/26258003/diff/5001/ui/keyboard/keyboar...
ui/keyboard/keyboard_util.cc:187: {"keyboard/layouts/system.html",
IDR_KEYBOARD_LAYOUTS_SYSTEM},
On 2013/10/08 14:58:06, bshe wrote:
> nit: alphabetical please

Done.

https://chromiumcodereview.appspot.com/26258003/diff/5001/ui/keyboard/resourc...
File ui/keyboard/resources/elements/kb-key-sequence.html (right):

https://chromiumcodereview.appspot.com/26258003/diff/5001/ui/keyboard/resourc...
ui/keyboard/resources/elements/kb-key-sequence.html:50: if(this.invert) {
On 2013/10/08 14:58:06, bshe wrote:
> Is the keycode still right after this key inverted?

I think it is. I tested it by inverting the qwerty keys and then running the VK
tests and it passed them all. If the key codes were messed up it wouldn't
display the correct character on key-up/key-down right?

[rsadam, 2013-10-08 21:38:00]

On 2013/10/08 21:36:14, rsadam wrote:
>
https://chromiumcodereview.appspot.com/26258003/diff/5001/ui/keyboard/keyboar...
> File ui/keyboard/keyboard_util.cc (right):
> 
>
https://chromiumcodereview.appspot.com/26258003/diff/5001/ui/keyboard/keyboar...
> ui/keyboard/keyboard_util.cc:187: {"keyboard/layouts/system.html",
> IDR_KEYBOARD_LAYOUTS_SYSTEM},
> On 2013/10/08 15:39:48, kevers wrote:
> > On 2013/10/08 14:58:06, bshe wrote:
> > > nit: alphabetical please
> > 
> > Also suggest using the name "system-qwerty.html" instead of system as there
> will
> > likely be several variants of the system keyboard based on locale.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/26258003/diff/5001/ui/keyboard/keyboar...
> ui/keyboard/keyboard_util.cc:187: {"keyboard/layouts/system.html",
> IDR_KEYBOARD_LAYOUTS_SYSTEM},
> On 2013/10/08 14:58:06, bshe wrote:
> > nit: alphabetical please
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/26258003/diff/5001/ui/keyboard/resourc...
> File ui/keyboard/resources/elements/kb-key-sequence.html (right):
> 
>
https://chromiumcodereview.appspot.com/26258003/diff/5001/ui/keyboard/resourc...
> ui/keyboard/resources/elements/kb-key-sequence.html:50: if(this.invert) {
> On 2013/10/08 14:58:06, bshe wrote:
> > Is the keycode still right after this key inverted?
> 
> I think it is. I tested it by inverting the qwerty keys and then running the
VK
> tests and it passed them all. If the key codes were messed up it wouldn't
> display the correct character on key-up/key-down right?

Edit: Further tests reveals that this is not the case. Working on fixing it now!

[rsadam, 2013-10-09 15:25:12]

Rebased to master, fixed keycodes and set as default layout when dealing with
passwords!

https://codereview.chromium.org/26258003/diff/5001/ui/keyboard/resources/elem...
File ui/keyboard/resources/elements/kb-key-sequence.html (right):

https://codereview.chromium.org/26258003/diff/5001/ui/keyboard/resources/elem...
ui/keyboard/resources/elements/kb-key-sequence.html:50: if(this.invert) {
Fixed!

[bshe, 2013-10-09 16:07:49]

lgtm with nits

https://codereview.chromium.org/26258003/diff/25001/ui/keyboard/resources/ele...
File ui/keyboard/resources/elements/kb-key-sequence.html (right):

https://codereview.chromium.org/26258003/diff/25001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-key-sequence.html:7: <polymer-element
name="kb-key-sequence" attributes="keys hintTexts keyCodes invert">
nit: this seems over 80 characters

https://codereview.chromium.org/26258003/diff/25001/ui/keyboard/resources/ele...
File ui/keyboard/resources/elements/kb-keyboard.html (right):

https://codereview.chromium.org/26258003/diff/25001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-keyboard.html:216: password: "system-qwerty"
not sure if you did that. But could you also check if the webui version of
keyboard can display system-qwerty correctly?
Also, a test for this would be nice.

[rsadam, 2013-10-09 19:04:45]

https://codereview.chromium.org/26258003/diff/25001/ui/keyboard/resources/ele...
File ui/keyboard/resources/elements/kb-key-sequence.html (right):

https://codereview.chromium.org/26258003/diff/25001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-key-sequence.html:7: <polymer-element
name="kb-key-sequence" attributes="keys hintTexts keyCodes invert">
On 2013/10/09 16:07:50, bshe wrote:
> nit: this seems over 80 characters 

Done.

https://codereview.chromium.org/26258003/diff/25001/ui/keyboard/resources/ele...
File ui/keyboard/resources/elements/kb-keyboard.html (right):

https://codereview.chromium.org/26258003/diff/25001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-keyboard.html:216: password: "system-qwerty"
As we discussed offline, disabled this for webui. In terms of a test; that'd be
blocked on another patch I'm working on. I'll add a TODO to add it once that's
in place.
On 2013/10/09 16:07:50, bshe wrote:
> not sure if you did that. But could you also check if the webui version of
> keyboard can display system-qwerty correctly?
> Also, a test for this would be nice.

[rsadam, 2013-10-09 19:05:39]

+sadrul for OWNERS

[sadrul, 2013-10-09 19:26:01]

non-JS/HTML code lgtm

[commit-bot: I haz the power, 2013-10-09 19:42:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsadam@chromium.org/26258003/41001

[bshe, 2013-10-09 19:42:54]

https://codereview.chromium.org/26258003/diff/41001/ui/keyboard/resources/ele...
File ui/keyboard/resources/elements/kb-keyboard.html (right):

https://codereview.chromium.org/26258003/diff/41001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-keyboard.html:249: inputTypeChanged:
function() {
we can probably just override inputType setter and move all the logic to the
setter?
something like:
set inputType(newValue) {
 ...
}

[rsadam, 2013-10-09 20:18:45]

https://codereview.chromium.org/26258003/diff/41001/ui/keyboard/resources/ele...
File ui/keyboard/resources/elements/kb-keyboard.html (right):

https://codereview.chromium.org/26258003/diff/41001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-keyboard.html:249: inputTypeChanged:
function() {
As we discussed offline, that will cause timing issues.
On 2013/10/09 19:42:55, bshe wrote:
> we can probably just override inputType setter and move all the logic to the
> setter?
> something like:
> set inputType(newValue) {
>  ...
> }

[commit-bot: I haz the power, 2013-10-10 17:35:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsadam@chromium.org/26258003/41001

[commit-bot: I haz the power, 2013-10-10 20:13:51]

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

[commit-bot: I haz the power, 2013-10-10 20:19:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsadam@chromium.org/26258003/41001

[commit-bot: I haz the power, 2013-10-10 22:34:42]

Change committed as 228031