For faked key events from virtual keyboard, the character is set but dom key is not set. In such ca…

For faked key events from virtual keyboard, the character is set but dom key is not set. In such case, forcibly set the dom key to unidentified.

BUG=443889
TEST=Verified on link_freon & clapper devices.

Committed: https://crrev.com/22428333277a7d9d956acb1af3cc70aab85157c3
Cr-Commit-Position: refs/heads/master@{#309352}

[Shu Chen, 2014-12-19 08:00:39]

shuchen@chromium.org changed reviewers:
+ kpschoedel@chromium.org, sadrul@chromium.org

[Shu Chen, 2014-12-19 08:00:39]

sadrul@/kpschoedel@, can you please review this cl? Thanks

[Shu Chen, 2014-12-19 08:29:21]

https://codereview.chromium.org/815773003/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/815773003/diff/1/ui/events/event.cc#newcode757
ui/events/event.cc:757: key_ = DomKey::UNIDENTIFIED;
Not sure whether it's more appropriate to set as DomKey::CHARACTER.

[kpschoedel, 2014-12-19 16:08:51]

Social LGTM.

https://codereview.chromium.org/815773003/diff/1/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/815773003/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_engine.cc:337: ch);
I think eventually we want something like:

    ui::DomKey key = ui::DomKey::NONE;
    base::char16 ch = 0;
    if (!event.key.empty() && event.key.size() < 4) {
      base::string16 key_char = base::UTF8ToUTF16(event.key);
      if (key_char.size() == 1) {
        key = ui::DomKey::CHARACTER;
        ch = key_char[0];
      }
    }
    if (key == ui::DomKey::NONE) {
      key = ui::KeycodeConverter::KeyStringToDomKey(event.key.c_str());
      if (key == ui::DomKey::NONE)
        key = ui::DomKey::UNIDENTIFIED;
    }
    ui::KeyEvent ui_event(
        type,
        key_code,
        ui::KeycodeConverter::CodeStringToDomCode(event.code.c_str()),
        flags,
        key,
        ch);

... but I don't think it's necessary to change it now. I've started working
through the rest of the 'incomplete' KeyEvent constructors, and I think we will
end up with a utility function for this, mapping a web DOM |key| string to a
(DomKey, character) pair.

https://codereview.chromium.org/815773003/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/815773003/diff/1/ui/events/event.cc#newcode757
ui/events/event.cc:757: key_ = DomKey::UNIDENTIFIED;
On 2014/12/19 08:29:21, Shu Chen wrote:
> Not sure whether it's more appropriate to set as DomKey::CHARACTER.

UNIDENTIFIED is fine for this CL, as it is defined as "unable to identify".
There is code in the Ozone xkbcommon interface that I will factor out for
non-Ozone to cover the special cases (e.g. character 8 -> DomKey::BACKSPACE) and
to cover non-character keys.

[sadrul, 2014-12-19 16:46:57]

lgtm

[kpschoedel, 2014-12-19 16:49:06]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-12-19 16:50:22]

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

[commit-bot: I haz the power, 2014-12-19 19:02:29]

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

[commit-bot: I haz the power, 2014-12-19 19:02:30]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Shu Chen, 2014-12-20 11:38:11]

https://codereview.chromium.org/815773003/diff/1/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/input_method_engine.cc (right):

https://codereview.chromium.org/815773003/diff/1/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_engine.cc:337: ch);
On 2014/12/19 16:08:51, kpschoedel wrote:
> I think eventually we want something like:
> 
>     ui::DomKey key = ui::DomKey::NONE;
>     base::char16 ch = 0;
>     if (!event.key.empty() && event.key.size() < 4) {
>       base::string16 key_char = base::UTF8ToUTF16(event.key);
>       if (key_char.size() == 1) {
>         key = ui::DomKey::CHARACTER;
>         ch = key_char[0];
>       }
>     }
>     if (key == ui::DomKey::NONE) {
>       key = ui::KeycodeConverter::KeyStringToDomKey(event.key.c_str());
>       if (key == ui::DomKey::NONE)
>         key = ui::DomKey::UNIDENTIFIED;
>     }
>     ui::KeyEvent ui_event(
>         type,
>         key_code,
>         ui::KeycodeConverter::CodeStringToDomCode(event.code.c_str()),
>         flags,
>         key,
>         ch);
> 
> ... but I don't think it's necessary to change it now. I've started working
> through the rest of the 'incomplete' KeyEvent constructors, and I think we
will
> end up with a utility function for this, mapping a web DOM |key| string to a
> (DomKey, character) pair.

ACK. Thanks & looking forward to the dom key is fully supported for non-ozone
platforms.

[Shu Chen, 2014-12-20 11:38:21]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-12-20 11:39:26]

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

[commit-bot: I haz the power, 2014-12-20 16:09:22]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-12-20 16:10:21]

Patchset 1 (id:??) landed as
https://crrev.com/22428333277a7d9d956acb1af3cc70aab85157c3
Cr-Commit-Position: refs/heads/master@{#309352}