Switch from text insertion to key press and release events on the virtual k…

Switch from text insertion to key press and release events on the virtual keyboard. First step in integration the virtual keyboard with IMEs.
BUG=257093, 257098
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221393

[kevers, 2013-07-25 07:32:56]

Hi Biao,

Can you please take a look at this CL.

[bshe, 2013-07-25 15:32:10]

On 2013/07/25 07:32:56, kevers wrote:
> Hi Biao,
> 
> Can you please take a look at this CL.

lgtm with some nits
But I dont know much about keyboard event code.

[bshe, 2013-07-25 15:33:05]

sorry. comments weren't included last time.

https://codereview.chromium.org/20145004/diff/4002/chrome/common/extensions/a...
File chrome/common/extensions/api/experimental_input_virtual_keyboard.json
(right):

https://codereview.chromium.org/20145004/diff/4002/chrome/common/extensions/a...
chrome/common/extensions/api/experimental_input_virtual_keyboard.json:32: {
"name": "type",
probably add a enum field here like this
(https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext...
It could give us a js console error when type is not "keyPressed" or
"keyReleased".

But you have error detection in keyboard_util for extension and webui parameter.
So this is not necessary, just thought it might be slightly clearer to have a js
console error.

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

https://codereview.chromium.org/20145004/diff/4002/ui/keyboard/keyboard_util....
ui/keyboard/keyboard_util.cc:37: uint16 ch =
ui::GetCharacterFromKeyCode(static_cast<ui::KeyboardCode>(i),
nit: indent is off here.

https://codereview.chromium.org/20145004/diff/4002/ui/keyboard/resources/elem...
File ui/keyboard/resources/elements/kb-keyboard.html (right):

https://codereview.chromium.org/20145004/diff/4002/ui/keyboard/resources/elem...
ui/keyboard/resources/elements/kb-keyboard.html:147: insertText(detail.char);
Here and the insertText below probably also need to convert to dispatchKeyEvent?
Or maybe this is intentional?

https://codereview.chromium.org/20145004/diff/4002/ui/keyboard/resources/elem...
ui/keyboard/resources/elements/kb-keyboard.html:240: }
Does this break multiple character keys like ".com"? Or maybe I understand
wrong?

[kevers, 2013-07-26 00:53:30]

https://codereview.chromium.org/20145004/diff/4002/chrome/common/extensions/a...
File chrome/common/extensions/api/experimental_input_virtual_keyboard.json
(right):

https://codereview.chromium.org/20145004/diff/4002/chrome/common/extensions/a...
chrome/common/extensions/api/experimental_input_virtual_keyboard.json:32: {
"name": "type",
On 2013/07/25 15:33:05, bshe wrote:
> probably add a enum field here like this
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext...
> It could give us a js console error when type is not "keyPressed" or
> "keyReleased".
> 
> But you have error detection in keyboard_util for extension and webui
parameter.
> So this is not necessary, just thought it might be slightly clearer to have a
js
> console error.

Added enum.

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

https://codereview.chromium.org/20145004/diff/4002/ui/keyboard/keyboard_util....
ui/keyboard/keyboard_util.cc:37: uint16 ch =
ui::GetCharacterFromKeyCode(static_cast<ui::KeyboardCode>(i),
On 2013/07/25 15:33:05, bshe wrote:
> nit: indent is off here.
Fixed.

https://codereview.chromium.org/20145004/diff/4002/ui/keyboard/resources/elem...
File ui/keyboard/resources/elements/kb-keyboard.html (right):

https://codereview.chromium.org/20145004/diff/4002/ui/keyboard/resources/elem...
ui/keyboard/resources/elements/kb-keyboard.html:147: insertText(detail.char);
On 2013/07/25 15:33:05, bshe wrote:
> Here and the insertText below probably also need to convert to
dispatchKeyEvent?
> Or maybe this is intentional?

Was leaving it for a followup CL, but trivial enough to change.  Converted to
use keyTyped.

https://codereview.chromium.org/20145004/diff/4002/ui/keyboard/resources/elem...
ui/keyboard/resources/elements/kb-keyboard.html:240: }
On 2013/07/25 15:33:05, bshe wrote:
> Does this break multiple character keys like ".com"? Or maybe I understand
> wrong?

Good catch.  Fixed.

[kevers, 2013-07-26 00:58:19]

+bryeung for ui/keyboard OWNERS
+asvitkine for tools/metrics OWNERS
+mpcomplete for chrome/browser/extensions OWNERS

[Alexei Svitkine (slow), 2013-07-26 02:07:56]

histograms.xml lgtm

[Matt Perry, 2013-07-26 18:30:35]

Is this the Ash re-implementation mentioned in 
https://code.google.com/p/chromium/issues/detail?id=128295 ?

If you're going to be adding this back, could you (in a followup CL) take it out
of the experimental namespace? Add it to _api_features.json and
_permission_features.json, restricted to the dev channel. See
https://codereview.chromium.org/20081002/ for an example of how to do that.

[Matt Perry, 2013-07-26 18:31:23]

Implementation LGTM. Please also make sure to go through API review:
http://go/newchromeapi

[kevers, 2013-07-29 04:24:14]

On 2013/07/26 18:30:35, Matt Perry wrote:
> Is this the Ash re-implementation mentioned in 
> https://code.google.com/p/chromium/issues/detail?id=128295 ?
> 
> If you're going to be adding this back, could you (in a followup CL) take it
out
> of the experimental namespace? Add it to _api_features.json and
> _permission_features.json, restricted to the dev channel. See
> https://codereview.chromium.org/20081002/ for an example of how to do that.

Yes, this is an Ash-reimplementation of the virtual keyboard.

Thanks for the tip.  Will add a followup to migrate from experimental to dev
channel.

[kevers, 2013-08-28 15:41:32]

This CL is currently on hold pending removal of the experimental namespace.

[kevers, 2013-09-03 22:21:21]

Ready for (re)review. 

mpcomplete, can you please do a sanity check on the extension API. A number of
changes from the earlier version.

bshe can you please take a second look as well.

+sadrul for ui/keyboard.

[Matt Perry, 2013-09-03 22:33:32]

lgtm

https://chromiumcodereview.appspot.com/20145004/diff/29001/chrome/browser/ext...
File chrome/browser/extensions/api/input/input.cc (right):

https://chromiumcodereview.appspot.com/20145004/diff/29001/chrome/browser/ext...
chrome/browser/extensions/api/input/input.cc:72:
EXTENSION_FUNCTION_VALIDATE(params->GetBoolean("shiftKey", &shift_modifier));
If you use the json schema compiler, you won't have to parse this yourself. See
http://dev.chromium.org/developers/design-documents/extensions/proposed-chang...
(search for api.gyp). The alarms API is a good example of how to use it.

https://chromiumcodereview.appspot.com/20145004/diff/29001/chrome/common/exte...
File chrome/common/extensions/api/virtual_keyboard_private.json (right):

https://chromiumcodereview.appspot.com/20145004/diff/29001/chrome/common/exte...
chrome/common/extensions/api/virtual_keyboard_private.json:18: "shiftKey":
{"type": "boolean", "optional": true, "description": "Whether or not the SHIFT
key is pressed."}
nit: add "Defaults to false." (I'm assuming)

[sadrul, 2013-09-03 22:43:43]

https://codereview.chromium.org/20145004/diff/29001/ui/keyboard/keyboard_ui_h...
File ui/keyboard/keyboard_ui_handler.cc (right):

https://codereview.chromium.org/20145004/diff/29001/ui/keyboard/keyboard_ui_h...
ui/keyboard/keyboard_ui_handler.cc:32: "insertText",
Is 'insertText' used anymore? If both 'insertText' and 'sendKeyEvent' are
available, what are the differences between the two? If 'insertText' is not used
anymore, does it make sense to remove this code?

[bshe, 2013-09-03 23:18:11]

lgtm

https://chromiumcodereview.appspot.com/20145004/diff/29001/ui/keyboard/resour...
File ui/keyboard/resources/elements/kb-key-codes.html (right):

https://chromiumcodereview.appspot.com/20145004/diff/29001/ui/keyboard/resour...
ui/keyboard/resources/elements/kb-key-codes.html:151: */
function names should start with a lower case letter? Or it doesn't matter? My
understanding is that only function constructor starts with upper case. But I
may miss something from the style guide.

https://chromiumcodereview.appspot.com/20145004/diff/29001/ui/keyboard/resour...
ui/keyboard/resources/elements/kb-key-codes.html:163: keyCode = state.keyCode;
nit: indent is off

[kevers, 2013-09-04 13:51:20]

https://codereview.chromium.org/20145004/diff/29001/ui/keyboard/keyboard_ui_h...
File ui/keyboard/keyboard_ui_handler.cc (right):

https://codereview.chromium.org/20145004/diff/29001/ui/keyboard/keyboard_ui_h...
ui/keyboard/keyboard_ui_handler.cc:32: "insertText",
On 2013/09/03 22:43:43, sadrul wrote:
> Is 'insertText' used anymore? If both 'insertText' and 'sendKeyEvent' are
> available, what are the differences between the two? If 'insertText' is not
used
> anymore, does it make sense to remove this code?

InsertText is still being used for multi-character buttons like ".com".  In this
case, we do not want the characters to be fed through IME where they could be
reinterpreted as part of a text composition.

https://codereview.chromium.org/20145004/diff/29001/ui/keyboard/resources/ele...
File ui/keyboard/resources/elements/kb-key-codes.html (right):

https://codereview.chromium.org/20145004/diff/29001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-key-codes.html:151: */
On 2013/09/03 23:18:11, bshe wrote:
> function names should start with a lower case letter? Or it doesn't matter? My
> understanding is that only function constructor starts with upper case. But I
> may miss something from the style guide.

Yes, it should have been lowercase.  Fixed.

https://codereview.chromium.org/20145004/diff/29001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-key-codes.html:163: keyCode = state.keyCode;
On 2013/09/03 23:18:11, bshe wrote:
> nit: indent is off

Fixed.

[sadrul, 2013-09-04 15:44:53]

ui/keyboard/ LGTM

https://codereview.chromium.org/20145004/diff/29001/ui/keyboard/keyboard_ui_h...
File ui/keyboard/keyboard_ui_handler.cc (right):

https://codereview.chromium.org/20145004/diff/29001/ui/keyboard/keyboard_ui_h...
ui/keyboard/keyboard_ui_handler.cc:32: "insertText",
On 2013/09/04 13:51:20, kevers wrote:
> On 2013/09/03 22:43:43, sadrul wrote:
> > Is 'insertText' used anymore? If both 'insertText' and 'sendKeyEvent' are
> > available, what are the differences between the two? If 'insertText' is not
> used
> > anymore, does it make sense to remove this code?
> 
> InsertText is still being used for multi-character buttons like ".com".  In
this
> case, we do not want the characters to be fed through IME where they could be
> reinterpreted as part of a text composition.

Ah, ok. Thanks for explaining.

https://codereview.chromium.org/20145004/diff/29001/ui/keyboard/keyboard_util.h
File ui/keyboard/keyboard_util.h (right):

https://codereview.chromium.org/20145004/diff/29001/ui/keyboard/keyboard_util...
ui/keyboard/keyboard_util.h:46: // value, and {shift_modifier| indicates if the
shift key is being virtually
s/{/|/

[kevers, 2013-09-04 18:01:14]

https://codereview.chromium.org/20145004/diff/29001/ui/keyboard/keyboard_util.h
File ui/keyboard/keyboard_util.h (right):

https://codereview.chromium.org/20145004/diff/29001/ui/keyboard/keyboard_util...
ui/keyboard/keyboard_util.h:46: // value, and {shift_modifier| indicates if the
shift key is being virtually
On 2013/09/04 15:44:54, sadrul wrote:
> s/{/|/

Done.

[commit-bot: I haz the power, 2013-09-04 18:01:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/20145004/45001

[commit-bot: I haz the power, 2013-09-04 18:02:11]

Failed to apply patch for ui/keyboard/resources/elements/kb-key.html:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/keyboard/resources/elements/kb-key.html
  Hunk #2 FAILED at 71.
  1 out of 2 hunks FAILED -- saving rejects to file
ui/keyboard/resources/elements/kb-key.html.rej

Patch:       ui/keyboard/resources/elements/kb-key.html
Index: ui/keyboard/resources/elements/kb-key.html
diff --git a/ui/keyboard/resources/elements/kb-key.html
b/ui/keyboard/resources/elements/kb-key.html
index
c37d70716668052b0a37e5374ec7dec09abb1017..936dabb4f5c9b46c909fbacbbacdf4576d33926e
100644
--- a/ui/keyboard/resources/elements/kb-key.html
+++ b/ui/keyboard/resources/elements/kb-key.html
@@ -39,7 +39,18 @@
        * Keys with a high weighting are wider than normal keys.
        * @type {number}
        */
-      weight: 1
+      weight: 1,
+
+      /**
+       * Returns a subset of the key attributes.
+       * @return {Object} Mapping of attributes for the key element.
+       */
+      PopulateDetails: function() {
+        var details = this.super();
+        details.keyCode = this.keyCode;
+        details.shiftModifier = this.shiftModifier;
+        return details;
+      },
     });
   </script>
 </polymer-element>
@@ -60,7 +71,7 @@
         }
         this.fire('enable-dbl', detail);
         this.fire('enable-sel');
-      }
+      },
     });
   </script>
 </polymer-element>

[commit-bot: I haz the power, 2013-09-04 18:22:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/20145004/50001

[commit-bot: I haz the power, 2013-09-04 18:26:45]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-04 18:54:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/20145004/50001

[commit-bot: I haz the power, 2013-09-05 08:19:48]

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

[commit-bot: I haz the power, 2013-09-05 11:34:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/20145004/50001

[commit-bot: I haz the power, 2013-09-05 13:04:17]

Change committed as 221393