Remove ui::KeycodeConverter::CodeToNativeKeycode().

Remove some |char*| functions from KeycodeConverter

Convert code to use enum DomCode rather than strings.

BUG=444048
R=wez@chromium.org,bbudge@chromium.org,bauerb@chromium.org,jcivelli@chromium.org,sadrul@chromium.org

Presubmit has failed to acknowledge the "LGTM" at https://codereview.chromium.org/1124963003/#msg29 so I'm adding this TBR as a workaround:

TBR=jcivelli

Committed: https://crrev.com/103db4435a5e181c741c0698dbfec37fd0dccd57
Cr-Commit-Position: refs/heads/master@{#330158}

[kpschoedel, 2015-05-05 19:11:42]

kpschoedel@chromium.org changed reviewers:
+ wez@chromium.org

[Wez, 2015-05-05 20:11:48]

https://codereview.chromium.org/1124963003/diff/1/chrome/browser/ui/blocked_c...
File chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc (right):

https://codereview.chromium.org/1124963003/diff/1/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc:49: #include
"ui/events/keycodes/dom4/keycode_converter.h"
For a follow-up CL: |code| is now in DOM3 - perhaps best to move dom3/ and dom4/
into a single dom/ folder now?

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

https://codereview.chromium.org/1124963003/diff/1/ui/events/event_unittest.cc...
ui/events/event_unittest.cc:352:
ui::KeycodeConverter::CodeStringToDomCode(kCodeForSpace));
nit: I don't think this ASSERT really adds anything; it's basically testing the
same thing that the EXPECT_EQ below is there to test, in effect.

https://codereview.chromium.org/1124963003/diff/1/ui/events/event_unittest.cc...
ui/events/event_unittest.cc:357:
ui::KeycodeConverter::DomCodeToNativeKeycode(kDomCodeForSpace));
This assertion is trivially true now!

https://codereview.chromium.org/1124963003/diff/1/ui/events/keycodes/dom4/key...
File ui/events/keycodes/dom4/keycode_converter.h (right):

https://codereview.chromium.org/1124963003/diff/1/ui/events/keycodes/dom4/key...
ui/events/keycodes/dom4/keycode_converter.h:48: static const char*
NativeKeycodeToCode(int native_keycode);
If we're removing |code| string to native keycode, what about removing this as
well?

https://codereview.chromium.org/1124963003/diff/1/ui/events/keycodes/dom4/key...
File ui/events/keycodes/dom4/keycode_converter_unittest.cc (right):

https://codereview.chromium.org/1124963003/diff/1/ui/events/keycodes/dom4/key...
ui/events/keycodes/dom4/keycode_converter_unittest.cc:61:
ui::KeycodeConverter::CodeStringToDomCode(entry->code);
You only need this because you're testing DomCode->native and
native->DomCodeString - if you also switch to native->DomCode->DomCodeString
flow then this test will be simpler, with no need to deal w/ the string form
(and you could then test the DomCode<->DomCodeString mapping separately).

https://codereview.chromium.org/1124963003/diff/1/ui/events/keycodes/dom4/key...
ui/events/keycodes/dom4/keycode_converter_unittest.cc:66:
entry->native_keycode));
nit: More readable to wrap after "code," and keep the NativeKeycodeToCode() call
on a single line?

[kpschoedel, 2015-05-05 20:48:50]

https://codereview.chromium.org/1124963003/diff/1/chrome/browser/ui/blocked_c...
File chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc (right):

https://codereview.chromium.org/1124963003/diff/1/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc:49: #include
"ui/events/keycodes/dom4/keycode_converter.h"
On 2015/05/05 20:11:48, Wez wrote:
> For a follow-up CL: |code| is now in DOM3 - perhaps best to move dom3/ and
dom4/
> into a single dom/ folder now?

OK.

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

https://codereview.chromium.org/1124963003/diff/1/ui/events/event_unittest.cc...
ui/events/event_unittest.cc:352:
ui::KeycodeConverter::CodeStringToDomCode(kCodeForSpace));
On 2015/05/05 20:11:48, Wez wrote:
> nit: I don't think this ASSERT really adds anything; it's basically testing
the
> same thing that the EXPECT_EQ below is there to test, in effect.

Done.

https://codereview.chromium.org/1124963003/diff/1/ui/events/event_unittest.cc...
ui/events/event_unittest.cc:357:
ui::KeycodeConverter::DomCodeToNativeKeycode(kDomCodeForSpace));
On 2015/05/05 20:11:48, Wez wrote:
> This assertion is trivially true now!

Done.

https://codereview.chromium.org/1124963003/diff/1/ui/events/keycodes/dom4/key...
File ui/events/keycodes/dom4/keycode_converter.h (right):

https://codereview.chromium.org/1124963003/diff/1/ui/events/keycodes/dom4/key...
ui/events/keycodes/dom4/keycode_converter.h:48: static const char*
NativeKeycodeToCode(int native_keycode);
On 2015/05/05 20:11:48, Wez wrote:
> If we're removing |code| string to native keycode, what about removing this as
> well?

I had left them because the uses were outside tests, but sure. Also removed
UsbKeycodeToCode, which was not used externally.

https://codereview.chromium.org/1124963003/diff/1/ui/events/keycodes/dom4/key...
File ui/events/keycodes/dom4/keycode_converter_unittest.cc (right):

https://codereview.chromium.org/1124963003/diff/1/ui/events/keycodes/dom4/key...
ui/events/keycodes/dom4/keycode_converter_unittest.cc:61:
ui::KeycodeConverter::CodeStringToDomCode(entry->code);
On 2015/05/05 20:11:48, Wez wrote:
> You only need this because you're testing DomCode->native and
> native->DomCodeString - if you also switch to native->DomCode->DomCodeString
> flow then this test will be simpler, with no need to deal w/ the string form
> (and you could then test the DomCode<->DomCodeString mapping separately).

Done. (DomCode<->DomCodeString is tested in KeycodeConverter.DomCode below)

https://codereview.chromium.org/1124963003/diff/1/ui/events/keycodes/dom4/key...
ui/events/keycodes/dom4/keycode_converter_unittest.cc:66:
entry->native_keycode));
On 2015/05/05 20:11:48, Wez wrote:
> nit: More readable to wrap after "code," and keep the NativeKeycodeToCode()
call
> on a single line?

Agreed — I liked the old clang-format rule better — but the line is gone now.

[kpschoedel, 2015-05-05 23:10:56]

https://codereview.chromium.org/1124963003/diff/1/ui/events/keycodes/dom4/key...
File ui/events/keycodes/dom4/keycode_converter.h (right):

https://codereview.chromium.org/1124963003/diff/1/ui/events/keycodes/dom4/key...
ui/events/keycodes/dom4/keycode_converter.h:48: static const char*
NativeKeycodeToCode(int native_keycode);
On 2015/05/05 20:11:48, Wez wrote:
> If we're removing |code| string to native keycode, what about removing this as
> well?

On further inspection, it looks like all the users of this, and their five
defining files content/renderer/pepper/usb_key_code_conversion*.*, can be
removed entirely now that blink::WebKeyboardEvent contains the DOM |code|. Will
test and update.

[kpschoedel, 2015-05-06 15:09:13]

kpschoedel@chromium.org changed reviewers:
+ bbudge@chromium.org

[kpschoedel, 2015-05-06 15:09:13]

+bbudge@ for content/renderer/pepper/*

[bbudge, 2015-05-06 15:28:07]

content/renderer/pepper LGTM

[Wez, 2015-05-06 23:37:28]

lgtm

https://codereview.chromium.org/1124963003/diff/40001/content/renderer/pepper...
File content/renderer/pepper/usb_key_code_conversion_win.cc (left):

https://codereview.chromium.org/1124963003/diff/40001/content/renderer/pepper...
content/renderer/pepper/usb_key_code_conversion_win.cc:19: scancode |= 0xe000;
nit: Initially I thought we were missing this logic in the new code, which has
it in GetScaneCodeFromLParam in event_win.cc; the comment on that function, and
on the related CodeForWindowsScanCode should ideally be updated to make clear
the form that our version of "windows scancodes" take (i.e. that we fold the
extended bit and OEM scancode together to get a PS/2-like scancode).

[kpschoedel, 2015-05-07 14:40:58]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2015-05-07 14:41:07]

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

[commit-bot: I haz the power, 2015-05-07 14:44:46]

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

[commit-bot: I haz the power, 2015-05-07 14:44:46]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kpschoedel, 2015-05-07 15:06:22]

The CQ bit was checked by kpschoedel@chromium.org

[kpschoedel, 2015-05-07 15:06:22]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1124963003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-07 15:06:47]

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

[commit-bot: I haz the power, 2015-05-07 15:11:07]

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

[commit-bot: I haz the power, 2015-05-07 15:11:08]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kpschoedel, 2015-05-15 16:07:38]

Patchset #4 (id:60001) has been deleted

[kpschoedel, 2015-05-15 16:08:27]

The CQ bit was checked by kpschoedel@chromium.org to run a CQ dry run

[kpschoedel, 2015-05-15 16:08:28]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1124963003/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-15 16:09:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124963003/80001

[commit-bot: I haz the power, 2015-05-15 16:58:43]

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

[commit-bot: I haz the power, 2015-05-15 16:58:44]

Dry run: This issue passed the CQ dry run.

[kpschoedel, 2015-05-15 18:02:12]

kpschoedel@chromium.org changed reviewers:
+ bauerb@chromium.org, jcivelli@chromium.org, sadrul@chromium.org

[kpschoedel, 2015-05-15 18:02:13]

+bauerb@ for chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc

+jcivelli@ for content/public/test/browser_test_utils.cc

+sadrul@ for ui/events/event_unittest.cc

[sadrul, 2015-05-15 18:03:01]

said file lgtm

[Bernhard Bauer, 2015-05-15 18:15:33]

blocked_content LGTM

[Jay Civelli, 2015-05-15 18:32:38]

LGTM for content/public/test/browser_test_utils.cc

On Fri, May 15, 2015 at 11:02 AM, <kpschoedel@chromium.org> wrote:

> +bauerb@ for
> chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc
>
> +jcivelli@ for content/public/test/browser_test_utils.cc
>
> +sadrul@ for ui/events/event_unittest.cc
>
> https://codereview.chromium.org/1124963003/
>

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

[kpschoedel, 2015-05-15 18:33:17]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2015-05-15 18:35:06]

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

[commit-bot: I haz the power, 2015-05-15 18:46:32]

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

[commit-bot: I haz the power, 2015-05-15 18:46:36]

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

[kpschoedel, 2015-05-15 19:03:46]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2015-05-15 19:07:04]

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

[commit-bot: I haz the power, 2015-05-15 19:14:06]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-05-15 19:14:57]

Patchset 4 (id:??) landed as
https://crrev.com/103db4435a5e181c741c0698dbfec37fd0dccd57
Cr-Commit-Position: refs/heads/master@{#330158}