Removing X11 native_event uses for key events.

Removing uses of X11 native key events.

BUG=380349
Committed: https://chromium.googlesource.com/chromium/src/+/bbb7ea7c66f18298079eea4a6b24834986f4a822

Committed: https://crrev.com/19537df7b11b6e634c16a3073a77b98db3d7b61f
Cr-Commit-Position: refs/heads/master@{#293520}

Committed: https://crrev.com/c6037c66e3487ea57c90090c084d37b5b6fda816
Cr-Commit-Position: refs/heads/master@{#295963}

[kpschoedel, 2014-08-17 21:35:59]

sadrul, could you take a look at the ui/event part before I add other reviewers?

Note: the failures in asan are not due to this CL; I ran it off master for
comparison here:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromeos_...

[sadrul, 2014-08-19 03:46:19]

Looks reasonable to me.

https://codereview.chromium.org/479873002/diff/1/ash/accelerators/key_hold_de...
File ash/accelerators/key_hold_detector.cc (right):

https://codereview.chromium.org/479873002/diff/1/ash/accelerators/key_hold_de...
ash/accelerators/key_hold_detector.cc:22: void DispatchPressedEvent(ui::KeyEvent
event,
const &

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

https://codereview.chromium.org/479873002/diff/1/ui/events/event.cc#newcode788
ui/events/event.cc:788: return (flags() & EF_NUMPAD_KEY) ? VKEY_NUMPAD0 :
VKEY_0;
EF_NUMPAD_KEY is set only on X11. Does that make a difference?

https://codereview.chromium.org/479873002/diff/1/ui/events/event.cc#newcode807
ui/events/event.cc:807: // Locatable codes not currently distinguished:
Should be a TODO (maybe with an accompanying crbug)

[kpschoedel, 2014-08-19 21:58:10]

https://codereview.chromium.org/479873002/diff/1/ash/accelerators/key_hold_de...
File ash/accelerators/key_hold_detector.cc (right):

https://codereview.chromium.org/479873002/diff/1/ash/accelerators/key_hold_de...
ash/accelerators/key_hold_detector.cc:22: void DispatchPressedEvent(ui::KeyEvent
event,
On 2014/08/19 03:46:19, sadrul wrote:
> const &

This gets passed to the non-const OnEventFromSource(), and base::Bind doesn't
allow a non-const reference, so I've just made it a pointer to avoid copying.

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

https://codereview.chromium.org/479873002/diff/1/ui/events/event.cc#newcode788
ui/events/event.cc:788: return (flags() & EF_NUMPAD_KEY) ? VKEY_NUMPAD0 :
VKEY_0;
On 2014/08/19 03:46:19, sadrul wrote:
> EF_NUMPAD_KEY is set only on X11. Does that make a difference?

It's only used for X11 / Ozone at present, and it's likely that the Ozone
revisions will make this cross-platform. I'll add a TODO here as well.

https://codereview.chromium.org/479873002/diff/1/ui/events/event.cc#newcode807
ui/events/event.cc:807: // Locatable codes not currently distinguished:
On 2014/08/19 03:46:19, sadrul wrote:
> Should be a TODO (maybe with an accompanying crbug)

I'll just delete this. There will probably never be distinct VKEY codes for
those keys, and they're definitely not needed for the
Windows-legacy-compatibility that this function is meant for.

[sadrul, 2014-08-21 18:26:17]

lgtm But please have one of the owners of ui/events/keycodes/ also take a look.

[kpschoedel, 2014-08-21 20:49:47]

+garykac@ — Could you take a look, or pass to wez@? Thanks.

[kpschoedel, 2014-08-22 17:19:34]

This will need major revision due to https://codereview.chromium.org/492863002
landing same-but-different changes.

[kpschoedel, 2014-08-29 21:14:49]

Sadrul, could you take another look at this CL (patch set 5)? The changes need
after r291301 are more than the usual rebase.

[sadrul, 2014-09-02 14:49:44]

lgtm

[kpschoedel, 2014-09-02 17:00:07]

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

[kpschoedel, 2014-09-02 17:00:08]

+sky@ for non- ui/events; selecting you because you previously reviewed
https://codereview.chromium.org/492863002 with related changes from mojo.

cc +erg@ in case you have additional comment on the mojo/ files.

[sky, 2014-09-02 18:23:15]

https://codereview.chromium.org/479873002/diff/160001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/479873002/diff/160001/ui/events/event.h#newco...
ui/events/event.h:641: base::char16 GetUnmodifiedText() const;
My initial reaction is that all of this is a mess. Do we really need all these
variants that are similar yet differ in some subtle way? Can't the subtleties be
moved to the call sites so that the code in event.h is clean and without
idiosyncrasies?

[kpschoedel, 2014-09-02 19:27:26]

https://codereview.chromium.org/479873002/diff/160001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/479873002/diff/160001/ui/events/event.h#newco...
ui/events/event.h:641: base::char16 GetUnmodifiedText() const;
On 2014/09/02 18:23:15, sky wrote:
> My initial reaction is that all of this is a mess. Do we really need all these
> variants that are similar yet differ in some subtle way? Can't the subtleties
be
> moved to the call sites so that the code in event.h is clean and without
> idiosyncrasies?

That can be done, but these correspond to repeated patterns. Historically there
have been ad-hoc transformations of keystroke and/or character events with
duplicate code, or worse, *near* duplicate code with no explanation, and I've
encountered at least one bug due to someone making a change that was not carried
through everywhere. On top of that, callers of ui::KeyEvent::key_code() have not
generally been clear about which subset of ui::KeyboardCode (e.g. VKEY_SHIFT vs
VKEY_[LR]SHIFT) they expect (or indeed superset ui::KeyboardCode union UCS-2),
with the meaning of key_code_ not being documented, and defined only by implied
Windows emulation.

So the attempt to clarify and unify this is the motivation for the additional
accessors here. My own preference is still to have them as ui::KeyEvent members.
As an alternate proposal, I'd suggest the common , repeated operations be
provided as functions under ui/events/keycodes, or at least some designated
single place, with other current duplication (e.g.
content::GetWindowsKeyCodeWithoutLocation() and
DevToolsEventForwarder::VirtualKeyCodeWithoutLocation() are the same function)
eventually migrated.

[Elliot Glaysher, 2014-09-02 19:31:06]

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

[Elliot Glaysher, 2014-09-02 19:31:07]

https://codereview.chromium.org/479873002/diff/160001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/479873002/diff/160001/ui/events/event.h#newco...
ui/events/event.h:641: base::char16 GetUnmodifiedText() const;
On 2014/09/02 19:27:26, kpschoedel wrote:
> On 2014/09/02 18:23:15, sky wrote:
> > My initial reaction is that all of this is a mess. Do we really need all
these
> > variants that are similar yet differ in some subtle way? Can't the
subtleties
> be
> > moved to the call sites so that the code in event.h is clean and without
> > idiosyncrasies?
> 
> That can be done, but these correspond to repeated patterns. Historically
there
> have been ad-hoc transformations of keystroke and/or character events with
> duplicate code, or worse, *near* duplicate code with no explanation, and I've
> encountered at least one bug due to someone making a change that was not
carried
> through everywhere. On top of that, callers of ui::KeyEvent::key_code() have
not
> generally been clear about which subset of ui::KeyboardCode (e.g. VKEY_SHIFT
vs
> VKEY_[LR]SHIFT) they expect (or indeed superset ui::KeyboardCode union UCS-2),
> with the meaning of key_code_ not being documented, and defined only by
implied
> Windows emulation.
> 
> So the attempt to clarify and unify this is the motivation for the additional
> accessors here. My own preference is still to have them as ui::KeyEvent
members.
> As an alternate proposal, I'd suggest the common , repeated operations be
> provided as functions under ui/events/keycodes, or at least some designated
> single place, with other current duplication (e.g.
> content::GetWindowsKeyCodeWithoutLocation() and
> DevToolsEventForwarder::VirtualKeyCodeWithoutLocation() are the same function)
> eventually migrated.

One thing to watch out for (which you might already know) is that the vkey
constants aren't actually the same on all platforms. (I found this out the hard
way.) I would assume that in the long term, we probably want to standardize on
the windows constants. (Which is what I did in
mojo/services/public/interfaces/input_events/input_key_codes.mojom.)

[kpschoedel, 2014-09-02 19:47:16]

https://codereview.chromium.org/479873002/diff/160001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/479873002/diff/160001/ui/events/event.h#newco...
ui/events/event.h:641: base::char16 GetUnmodifiedText() const;
On 2014/09/02 19:31:07, Elliot Glaysher wrote:
> One thing to watch out for (which you might already know) is that the vkey
> constants aren't actually the same on all platforms. (I found this out the
hard
> way.) I would assume that in the long term, we probably want to standardize on
> the windows constants. (Which is what I did in
> mojo/services/public/interfaces/input_events/input_key_codes.mojom.)

I suspect that's an accidental drift, and looking into that and merging
ui/events/keycodes/keyboard_codes_* is on the list of things I want to do, but
really I'd like it better if they could be defined in just one place… blink and
pdfium have their own copies, too.

[sky, 2014-09-02 21:43:01]

How many of these new functions are used in more than one call site?

On Tue, Sep 2, 2014 at 12:27 PM,  <kpschoedel@chromium.org> wrote:
>
> https://codereview.chromium.org/479873002/diff/160001/ui/events/event.h
> File ui/events/event.h (right):
>
>
https://codereview.chromium.org/479873002/diff/160001/ui/events/event.h#newco...
> ui/events/event.h:641: base::char16 GetUnmodifiedText() const;
> On 2014/09/02 18:23:15, sky wrote:
>>
>> My initial reaction is that all of this is a mess. Do we really need
>
> all these
>>
>> variants that are similar yet differ in some subtle way? Can't the
>
> subtleties be
>>
>> moved to the call sites so that the code in event.h is clean and
>
> without
>>
>> idiosyncrasies?
>
>
> That can be done, but these correspond to repeated patterns.
> Historically there have been ad-hoc transformations of keystroke and/or
> character events with duplicate code, or worse, *near* duplicate code
> with no explanation, and I've encountered at least one bug due to
> someone making a change that was not carried through everywhere. On top
> of that, callers of ui::KeyEvent::key_code() have not generally been
> clear about which subset of ui::KeyboardCode (e.g. VKEY_SHIFT vs
> VKEY_[LR]SHIFT) they expect (or indeed superset ui::KeyboardCode union
> UCS-2), with the meaning of key_code_ not being documented, and defined
> only by implied Windows emulation.
>
> So the attempt to clarify and unify this is the motivation for the
> additional accessors here. My own preference is still to have them as
> ui::KeyEvent members. As an alternate proposal, I'd suggest the common ,
> repeated operations be provided as functions under ui/events/keycodes,
> or at least some designated single place, with other current duplication
> (e.g. content::GetWindowsKeyCodeWithoutLocation() and
> DevToolsEventForwarder::VirtualKeyCodeWithoutLocation() are the same
> function) eventually migrated.
>
> https://codereview.chromium.org/479873002/

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

[kpschoedel, 2014-09-02 22:31:15]

On 2014/09/02 21:43:01, sky wrote:
> How many of these new functions are used in more than one call site?

GetText() and GetUnmodifiedText() each have two callers, in this CL:
- content/browser/renderer_host/web_input_event_aura.cc
- mojo/services/public/cpp/input_events/lib/input_events_type_converters.cc 

GetLocatedWindowsKeyboardCode() has three callers in this CL:
- content/browser/renderer_host/web_input_event_aura.cc
- mojo/services/public/cpp/input_events/lib/input_events_type_converters.cc 
- mojo/services/native_viewport/platform_viewport_x11.cc

GetConflatedWindowsKeyCode() has one caller in this CL, and another location
with the same logic that I was going to submit separately, since it's not
covered by the subject of this CL ("Removing X11 native_event uses for key
events"):
- mojo/services/public/cpp/input_events/lib/input_events_type_converters.cc
- content/browser/renderer_host/render_widget_host_view_aura.cc
(The name comes from the DOM spec term “conflated model”.)

[sky, 2014-09-02 22:33:46]

*SIGH* LGTM

[kpschoedel, 2014-09-03 13:47:08]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-09-03 13:48:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/479873002/160001

[commit-bot: I haz the power, 2014-09-03 15:08:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-09-03 15:17:54]

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

[commit-bot: I haz the power, 2014-09-03 15:17:55]

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

[kpschoedel, 2014-09-03 15:26:44]

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

[kpschoedel, 2014-09-03 15:26:44]

+jamesr for content/renderer/render_view_browsertest.cc

[jamesr, 2014-09-03 17:33:37]

lgtm

[kpschoedel, 2014-09-03 17:34:22]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-09-03 17:35:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/479873002/160001

[commit-bot: I haz the power, 2014-09-03 17:49:07]

Committed patchset #6 (id:160001) as bbb7ea7c66f18298079eea4a6b24834986f4a822

[dcheng, 2014-09-04 03:50:17]

A revert of this CL (patchset #6 id:160001) has been created in
https://codereview.chromium.org/536313002/ by dcheng@chromium.org.

The reason for reverting is: Causing check_licenses to fail on
android_clang_dbg_recipe trybot:
The following files are whitelisted unnecessarily. You must remove the following
files from the whitelist.
content/browser/renderer_host/web_input_event_aurax11.cc
The following files are whitelisted, but do not exist.
content/browser/renderer_host/web_input_event_aurax11.cc.

[kpschoedel, 2014-09-04 17:27:51]

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

[kpschoedel, 2014-09-04 17:27:52]

+boliu@ for android_webview/tools/third_party_files_whitelist.txt

[boliu, 2014-09-04 17:34:47]

On 2014/09/04 17:27:52, kpschoedel wrote:
> +boliu@ for android_webview/tools/third_party_files_whitelist.txt

lgtm

Gonna file a bug for
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...,
check license failed, but bot is green, wat?

[kpschoedel, 2014-09-04 17:38:39]

On 2014/09/04 17:34:47, boliu wrote: 
> Gonna file a bug for
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...,
> check license failed, but bot is green, wat?

The CL was reverted to keep the bot green.

[kpschoedel, 2014-09-04 17:38:46]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-09-04 17:49:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/479873002/180001

[boliu, 2014-09-04 18:28:22]

On 2014/09/04 17:38:39, kpschoedel wrote:
> On 2014/09/04 17:34:47, boliu wrote: 
> > Gonna file a bug for
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...,
> > check license failed, but bot is green, wat?
> 
> The CL was reverted to keep the bot green.

That link is the try job on Patch Set 6. If you click on that link you can see
that check_license step failed, but the bot is green, so cq proceeded to land a
breaking change.

Filed at crbug.com/410904

[commit-bot: I haz the power, 2014-09-04 19:38:20]

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

[commit-bot: I haz the power, 2014-09-04 19:38:22]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kpschoedel, 2014-09-05 13:51:04]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-09-05 13:51:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/479873002/180001

[commit-bot: I haz the power, 2014-09-05 14:53:46]

Committed patchset #7 (id:180001) as 13d90b146a400156427243d05159cadd8d72c0b9

[arv (Not doing code reviews), 2014-09-08 15:38:37]

A revert of this CL (patchset #7 id:180001) has been created in
https://codereview.chromium.org/548303003/ by arv@chromium.org.

The reason for reverting is: Broke Android Tests (dbg)

C  427.575s Main  [FAIL]
org.chromium.content.browser.input.ImeTest#testKeyCodesWhileComposingText:
C  427.575s Main  junit.framework.AssertionFailedError
C  427.575s Main  	at
org.chromium.content.browser.input.ImeTest.assertUpdateStateCall(ImeTest.java:694)
C  427.575s Main  	at
org.chromium.content.browser.input.ImeTest.testKeyCodesWhileComposingText(ImeTest.java:410)
C  427.575s Main  	at java.lang.reflect.Method.invokeNative(Native Method)
C  427.575s Main  	at
android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
C  427.575s Main  	at
android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
C  427.575s Main  	at
android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
C  427.575s Main  	at
org.chromium.content_shell_apk.ContentShellTestBase.runTest(ContentShellTestBase.java:227)
C  427.575s Main  	at
android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191)
C  427.575s Main  	at
android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176)
C  427.575s Main  	at
android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554)
C  427.575s Main  	at
android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701).

[kpschoedel, 2014-09-08 16:39:37]

On 2014/09/08 15:38:37, arv wrote:
> A revert of this CL (patchset #7 id:180001) has been created in
> https://codereview.chromium.org/548303003/ by mailto:arv@chromium.org.
> 
> The reason for reverting is: Broke Android Tests (dbg)

Could you link to the failing run?

[commit-bot: I haz the power, 2014-09-10 03:26:41]

Patchset 7 (id:??) landed as
https://crrev.com/8f906f10b340299d817dfea06e4100adf26ea914
Cr-Commit-Position: refs/heads/master@{#293166}

[commit-bot: I haz the power, 2014-09-10 03:39:35]

Patchset 7 (id:??) landed as
https://crrev.com/19537df7b11b6e634c16a3073a77b98db3d7b61f
Cr-Commit-Position: refs/heads/master@{#293520}

[kpschoedel, 2014-09-22 13:41:23]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-09-22 13:41:40]

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

[commit-bot: I haz the power, 2014-09-22 13:59:30]

Committed patchset #7 (id:180001) as 6d0fce06c609fdb1504a89a6ffe78ab608faa982

[commit-bot: I haz the power, 2014-09-22 14:00:16]

Patchset 7 (id:??) landed as
https://crrev.com/c6037c66e3487ea57c90090c084d37b5b6fda816
Cr-Commit-Position: refs/heads/master@{#295963}

[kpschoedel, 2014-09-22 14:06:57]

On 2014/09/22 14:00:16, I haz the power (commit-bot) wrote:
> Patchset 7 (id:??) landed as
> https://crrev.com/c6037c66e3487ea57c90090c084d37b5b6fda816
> Cr-Commit-Position: refs/heads/master@{#295963}

I have not seen additional IME failures on Android vs current master in manual
tests; waiting to see if bots not selectable here find otherwise.