Add support for DOM3 KeyboardEvent keycode

Embedder API for accessing DOM3 code value based on dom code value

Sets the domCode value to the WebKeyboard event.
Blink then using embedder API call to converts a dom code to a DOM code value.

DOM3 code value are defined in keyboard_converer_data.h, where it maps the native value to the DOM code value. On blink side, it sends dom key value to fetch DOM3 code value.

Includes changes to the eventsender as previously it used windowsVirtualKeyCode. This has been replaced by code string, which is used to find equivalent domCode value.

BUG=227231
R=garykac, Wez

Committed: https://crrev.com/565c80c51413690b152b540f3b8dca9cb0abfdb5
Cr-Commit-Position: refs/heads/master@{#315013}

[Habib Virji, 2014-10-16 15:30:20]

To support blink changes https://codereview.chromium.org/663523002/

[Habib Virji, 2014-10-20 08:18:52]

habib.virji@samsung.com changed reviewers:
+ haraken@chromium.org, jamesr@chromium.org, tkent@chromium.org

[Habib Virji, 2014-10-22 15:27:14]

@garykac: Is change in the above mechanism of using usb code okay??

[Habib Virji, 2014-10-24 17:01:39]

On 2014/10/22 15:27:14, Habib Virji wrote:
> @garykac: Is change in the above mechanism of using usb code okay??
This change is not needed any more

[Habib Virji, 2014-10-27 16:29:12]

habib.virji@samsung.com changed reviewers:
+ wez@chromium.org
- haraken@chromium.org, jamesr@chromium.org, tkent@chromium.org

[Habib Virji, 2014-10-27 16:29:12]

habib.virji@samsung.com changed required reviewers:
+ garykac@chromium.org

[Habib Virji, 2014-10-27 16:48:15]

This CL has been updated based on comments from Wez and is required now.

[apavlov, 2014-11-12 08:53:24]

apavlov@chromium.org changed reviewers:
+ apavlov@chromium.org

[apavlov, 2014-11-12 08:53:24]

Any updates on this?

[apavlov, 2014-11-12 08:56:32]

https://codereview.chromium.org/658183002/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/658183002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl.cc:1789: int native) {
This seems to fit 80 chars unwrapped?

https://codereview.chromium.org/658183002/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/658183002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl.h:381: virtual const char*
getKeyboardDOMCodeValue(int nativeValue);
The style guide does not seem to honor the "get" prefix. keyboardDOMCodeValue()
should do fine.

https://codereview.chromium.org/658183002/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/event_sender.cc (right):

https://codereview.chromium.org/658183002/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/event_sender.cc:1273: } else if ("scrollLock"
== code_str) {
is this a drive-by change?

[Habib Virji, 2014-11-13 15:03:50]

@apavlov: Will update your changes. Kevin was doing some changes on the

[Habib Virji, 2014-11-13 15:04:24]

On 2014/11/13 15:03:50, Habib Virji wrote:
> @apavlov: Will update your changes. Kevin was doing some changes on the
chromium side. I am just waiting for that CL to land. Once done will push in for
my changes.

[apavlov, 2014-11-13 15:30:57]

On 2014/11/13 15:04:24, Habib Virji wrote:
> On 2014/11/13 15:03:50, Habib Virji wrote:
> > @apavlov: Will update your changes. Kevin was doing some changes on the
> chromium side. I am just waiting for that CL to land. Once done will push in
for
> my changes.

Thanks, Habib. Looking forward to this getting fixed.

[Habib Virji, 2014-11-24 15:13:10]

Thanks apavlov, have addressed your comments. Blink side is updated too. Hoping
for Wez or gary to have a look as chrome side changes are landed.

https://codereview.chromium.org/658183002/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/658183002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl.cc:1789: int native) {
On 2014/11/12 08:56:32, apavlov wrote:
> This seems to fit 80 chars unwrapped?

Done.

https://codereview.chromium.org/658183002/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/658183002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl.h:381: virtual const char*
getKeyboardDOMCodeValue(int nativeValue);
On 2014/11/12 08:56:32, apavlov wrote:
> The style guide does not seem to honor the "get" prefix.
keyboardDOMCodeValue()
> should do fine.

Done.

https://codereview.chromium.org/658183002/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/event_sender.cc (right):

https://codereview.chromium.org/658183002/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/event_sender.cc:1273: } else if ("scrollLock"
== code_str) {
Actually these changes are not needed any more, it is useful for testing but not
useful much. Removing them.

[apavlov, 2014-12-01 12:50:50]

A small nit: the first two issue Description lines essentially say the same
thing. Please retain only one of those and copy the issue Subject as the first
line of the Description.

[Habib Virji, 2014-12-01 13:16:38]

habib.virji@samsung.com changed required reviewers:
- garykac@chromium.org

[Habib Virji, 2014-12-01 13:17:38]

On 2014/12/01 at 12:50:50, apavlov wrote:
> A small nit: the first two issue Description lines essentially say the same
thing. Please retain only one of those and copy the issue Subject as the first
line of the Description.

Thanks. Done.

[Wez, 2014-12-02 03:59:44]

On 2014/12/01 13:17:38, Habib Virji wrote:
> On 2014/12/01 at 12:50:50, apavlov wrote:
> > A small nit: the first two issue Description lines essentially say the same
> thing. Please retain only one of those and copy the issue Subject as the first
> line of the Description.
> 
> Thanks. Done.

The description text doesn't really describe what _this_ CL does; can we update
it to reflect that this is the Chromium-side implementation of the embedder API
that Blink will call to convert a native code to a DOM code value.

[Wez, 2014-12-02 04:03:14]

https://codereview.chromium.org/658183002/diff/40001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/658183002/diff/40001/content/renderer/render_...
content/renderer/render_view_impl.cc:1825: return
ui::KeycodeConverter::NativeKeycodeToCode(native);
Two-space indent for the body of the method.

https://codereview.chromium.org/658183002/diff/40001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/658183002/diff/40001/content/renderer/render_...
content/renderer/render_view_impl.h:385: virtual const char*
keyboardDOMCodeValue(int nativeValue);
Is |int| going to be large enough to hold a DomKey value? I think they are
defined with 32-bit values.

[Wez, 2014-12-02 04:04:11]

Adding Kevin, FHI

[Habib Virji, 2014-12-02 12:07:04]

habib.virji@samsung.com changed reviewers:
+ dgozman@chromium.org

[Habib Virji, 2014-12-02 12:07:46]

https://codereview.chromium.org/658183002/diff/40001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/658183002/diff/40001/content/renderer/render_...
content/renderer/render_view_impl.cc:1825: return
ui::KeycodeConverter::NativeKeycodeToCode(native);
On 2014/12/02 04:03:14, Wez wrote:
> Two-space indent for the body of the method.

Done.

https://codereview.chromium.org/658183002/diff/40001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/658183002/diff/40001/content/renderer/render_...
content/renderer/render_view_impl.h:385: virtual const char*
keyboardDOMCodeValue(int nativeValue);
On 2014/12/02 at 04:03:14, Wez wrote:
> Is |int| going to be large enough to hold a DomKey value? I think they are
defined with 32-bit values.

It is based on the value from nativeVirtualKeyCode(), and it is int. Perhaps,
needs updating.

[Habib Virji, 2014-12-30 17:10:00]

habib.virji@samsung.com changed reviewers:
+ rbyers@chromium.org
- dgozman@chromium.org

[apavlov, 2015-01-02 09:17:23]

The CQ bit was checked by apavlov@chromium.org

[apavlov, 2015-01-02 09:17:30]

The CQ bit was unchecked by apavlov@chromium.org

[Habib Virji, 2015-01-26 11:50:42]

habib.virji@samsung.com changed reviewers:
+ jeremy@chromium.org
- apavlov@chromium.org, rbyers@chromium.org

[Habib Virji, 2015-01-26 11:57:59]

@jeremy: Can you please review changes for
content/renderer/render_view_impl.h/cc. These changes are for corresponding
Blink changes: https://codereview.chromium.org/663523002/. Adds two embedder API
to access DOM code value and DOM code enum.

@sadrul: content/browser/renderer_host/web_input_event_aura.cc. Change is to
assign WebKeyboardEvent, domCode, to set Dom code enum. 

@rbyers: These changes are to assign domCode value for test purpose,
content/shell/renderer/test_runner/event_sender.cc

[Habib Virji, 2015-01-26 11:58:20]

habib.virji@samsung.com changed reviewers:
+ rbyers@chromium.org, sadrul@chromium.org

[Habib Virji, 2015-01-26 12:47:50]

habib.virji@samsung.com changed reviewers:
+ jamesr@chromium.org, piman@chromium.org
- jeremy@chromium.org

[Habib Virji, 2015-01-26 12:48:46]

@piman/jamesr: Can you please review changes for
content/renderer/render_view_impl.h/cc. These changes are for corresponding
Blink changes: https://codereview.chromium.org/663523002/. Adds two embedder API
to access DOM code value and DOM code enum.

[jamesr, 2015-01-26 20:35:47]

jamesr@chromium.org changed reviewers:
- jamesr@chromium.org

[jamesr, 2015-01-26 20:35:48]

I'm not an owner for this code (any more)

[Habib Virji, 2015-01-27 10:10:07]

habib.virji@samsung.com changed reviewers:
+ jochen@chromium.org

[piman, 2015-01-28 01:18:04]

https://codereview.chromium.org/658183002/diff/120001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/658183002/diff/120001/content/renderer/render...
content/renderer/render_view_impl.h:365: virtual int domEnumFromCodeString(const
blink::WebString& codeString);
Why do these need to be virtual members of WebViewImpl? They don't access state,
they could be static.

https://codereview.chromium.org/658183002/diff/120001/content/shell/renderer/...
File content/shell/renderer/test_runner/event_sender.cc (right):

https://codereview.chromium.org/658183002/diff/120001/content/shell/renderer/...
content/shell/renderer/test_runner/event_sender.cc:1369: event_down.domCode =
static_cast<long>(
is domCode supposed to be long or int? We should be consistent.

[Habib Virji, 2015-01-28 10:02:11]

Thanks piman, updated event_sender with correct cast.

Regarding using static instead of virtual, perhaps it needs to be virtual in
render_view_impl.cc. As explained below.

https://codereview.chromium.org/658183002/diff/120001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/658183002/diff/120001/content/renderer/render...
content/renderer/render_view_impl.h:365: virtual int domEnumFromCodeString(const
blink::WebString& codeString);
No they do. 

Both functions are trying to access value defined in
ui/events/keycodes/dom4/keycode_converter_data.h. They are kind of helper
function for the values defined on the chromium side to be passed to the blink
side.

https://codereview.chromium.org/658183002/diff/120001/content/shell/renderer/...
File content/shell/renderer/test_runner/event_sender.cc (right):

https://codereview.chromium.org/658183002/diff/120001/content/shell/renderer/...
content/shell/renderer/test_runner/event_sender.cc:1369: event_down.domCode =
static_cast<long>(
It is supposed to be int.

[piman, 2015-01-28 18:17:14]

On Wed, Jan 28, 2015 at 2:02 AM, <habib.virji@samsung.com> wrote:

> Thanks piman, updated event_sender with correct cast.
>
> Regarding using static instead of virtual, perhaps it needs to be virtual
> in
> render_view_impl.cc. As explained below.
>
>
> https://codereview.chromium.org/658183002/diff/120001/
> content/renderer/render_view_impl.h
> File content/renderer/render_view_impl.h (right):
>
> https://codereview.chromium.org/658183002/diff/120001/
> content/renderer/render_view_impl.h#newcode365
> content/renderer/render_view_impl.h:365: virtual int
> domEnumFromCodeString(const blink::WebString& codeString);
> No they do.
>
> Both functions are trying to access value defined in
> ui/events/keycodes/dom4/keycode_converter_data.h. They are kind of
> helper function for the values defined on the chromium side to be passed
> to the blink side.


I understand it needs to be on the chromium side, but why does it have to
be a member function on RenderViewImpl?
It doesn't access any field from RenderViewImpl. The returned values don't
depend on which RenderView this is talking to.

>
>
> https://codereview.chromium.org/658183002/diff/120001/
> content/shell/renderer/test_runner/event_sender.cc
> File content/shell/renderer/test_runner/event_sender.cc (right):
>
> https://codereview.chromium.org/658183002/diff/120001/
> content/shell/renderer/test_runner/event_sender.cc#newcode1369
> content/shell/renderer/test_runner/event_sender.cc:1369:
> event_down.domCode = static_cast<long>(
> It is supposed to be int.
>
> https://codereview.chromium.org/658183002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2015-01-29 00:51:02]

https://codereview.chromium.org/658183002/diff/120001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/658183002/diff/120001/content/renderer/render...
content/renderer/render_view_impl.h:365: virtual int domEnumFromCodeString(const
blink::WebString& codeString);
On 2015/01/28 10:02:11, Habib Virji wrote:
> No they do. 
> 
> Both functions are trying to access value defined in
> ui/events/keycodes/dom4/keycode_converter_data.h. They are kind of helper
> function for the values defined on the chromium side to be passed to the blink
> side.
> 

These are embedder-provided APIs on the WebViewClient - the key event handling
code in Blink needs the embedder to provide them and WebViewClient ended up
being the cleanest place to do that, IIUC.

[piman, 2015-01-29 00:56:41]

On Wed, Jan 28, 2015 at 4:51 PM, <wez@chromium.org> wrote:

>
> https://codereview.chromium.org/658183002/diff/120001/
> content/renderer/render_view_impl.h
> File content/renderer/render_view_impl.h (right):
>
> https://codereview.chromium.org/658183002/diff/120001/
> content/renderer/render_view_impl.h#newcode365
> content/renderer/render_view_impl.h:365: virtual int
> domEnumFromCodeString(const blink::WebString& codeString);
> On 2015/01/28 10:02:11, Habib Virji wrote:
>
>> No they do.
>>
>
>  Both functions are trying to access value defined in
>> ui/events/keycodes/dom4/keycode_converter_data.h. They are kind of
>>
> helper
>
>> function for the values defined on the chromium side to be passed to
>>
> the blink
>
>> side.
>>
>
>
> These are embedder-provided APIs on the WebViewClient - the key event
> handling code in Blink needs the embedder to provide them and
> WebViewClient ended up being the cleanest place to do that, IIUC.
>

Given that RenderViewImpl is meant to be eventually removed, I would prefer
if we found another place, e.g. on BlinkPlatformImpl / blink::Platform.

Antoine


>
> https://codereview.chromium.org/658183002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Habib Virji, 2015-01-29 11:16:48]

Thanks Antoine, code has been updated to use blink_platform_impl and does not
use render_view_impl.

[piman, 2015-01-29 19:25:50]

lgtm

[Habib Virji, 2015-02-06 09:15:20]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2015-02-06 09:15:34]

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

[commit-bot: I haz the power, 2015-02-06 09:30:48]

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

[commit-bot: I haz the power, 2015-02-06 09:30:50]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Habib Virji, 2015-02-06 11:21:05]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2015-02-06 11:22:09]

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

[commit-bot: I haz the power, 2015-02-06 12:27:15]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-02-06 12:29:21]

Patchset 13 (id:??) landed as
https://crrev.com/565c80c51413690b152b540f3b8dca9cb0abfdb5
Cr-Commit-Position: refs/heads/master@{#315013}