Remove EF_FUNCTION_KEY and EF_NUMPAD_KEY.

Remove EF_FUNCTION_KEY and EF_NUMPAD_KEY.

These EventFlags are redundant now that DomCode exists, and were not
always set correctly or kept in sync.

Committed: https://crrev.com/8226fd15e53b122f531b3b1816b3e080bd8ef011
Cr-Commit-Position: refs/heads/master@{#328543}

[kpschoedel, 2015-04-30 20:46:55]

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

[commit-bot: I haz the power, 2015-04-30 20:48:28]

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

[commit-bot: I haz the power, 2015-04-30 22:13:56]

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

[commit-bot: I haz the power, 2015-04-30 22:13:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kpschoedel, 2015-05-01 15:27:32]

Patchset #5 (id:80001) has been deleted

[kpschoedel, 2015-05-01 15:27:44]

Patchset #4 (id:60001) has been deleted

[kpschoedel, 2015-05-01 15:27:52]

Patchset #3 (id:40001) has been deleted

[kpschoedel, 2015-05-01 15:28:03]

Patchset #2 (id:20001) has been deleted

[kpschoedel, 2015-05-01 15:56:22]

Patchset #2 (id:100001) has been deleted

[kpschoedel, 2015-05-01 16:13:16]

kpschoedel@chromium.org changed reviewers:
+ derat@chromium.org, sadrul@chromium.org, wez@chromium.org

[kpschoedel, 2015-05-01 16:13:18]

+derat@ for chrome/browser/chromeos/events/event_rewriter.*
+wez@ for ui/events/keycodes/*
+sadrul@ for the rest

https://codereview.chromium.org/1120813002/diff/1/chrome/browser/chromeos/eve...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/1120813002/diff/1/chrome/browser/chromeos/eve...
chrome/browser/chromeos/events/event_rewriter.cc:714: static const struct
NumPadRemapping {
I'm in the process of revising this file to handle DomCode and DomKey correctly,
so this change will itself be replaced soon. Actually, since nothing in
RewriteNumPadKeys depends on settings, I suspect it can be removed in favour of
generating the desired event in the first place (as already happens under
Ozone).

https://codereview.chromium.org/1120813002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/1120813002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/events/event_rewriter.cc:714: static const struct
NumPadRemapping {
I'm in the process of revising this file to handle DomCode and DomKey correctly,
so this change will itself be replaced soon. Actually, since nothing in
RewriteNumPadKeys depends on settings, I suspect it can be removed entirely in
favour of generating the desired event in the first place (as already happens
under Ozone).

https://codereview.chromium.org/1120813002/diff/120001/content/browser/render...
File content/browser/renderer_host/web_input_event_aura_unittest.cc (right):

https://codereview.chromium.org/1120813002/diff/120001/content/browser/render...
content/browser/renderer_host/web_input_event_aura_unittest.cc:27: {
Added non-X11 tests to this file.

https://codereview.chromium.org/1120813002/diff/120001/ui/events/blink/blink_...
File ui/events/blink/blink_event_util.cc (left):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/blink/blink_...
ui/events/blink/blink_event_util.cc:181: modifiers |=
blink::WebInputEvent::IsKeyPad;
There was a lurking bug here — see the (ex-) definition of EF_NUMPAD_KEY and the
definition of EF_IS_DOUBLE_CLICK.

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter.h (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter.h:19: enum class DomCodeLocation {
STANDARD, LEFT, RIGHT, NUMPAD };
I know the spec calls these DOM_KEY_LOCATION_STANDARD, etc., but DomKeyLocation
would be confusing here because it's derived from a DomCode rather than a
DomKey.

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter_data.h (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter_data.h:229: USB_KEYMAP(0x070085,
0x0081, 0x0000, 0x005f, "NumpadComma", NUMPAD_COMMA),
WebInputEventAuraTest uses this under X11.

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter_unittest.cc (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter_unittest.cc:21: const size_t
kExpectedMappedKeyCount = 168;
I hate this test.

[Daniel Erat, 2015-05-01 17:02:02]

rubberstamp lgtm for c/b/chromeos, but someone with more state about this code
(sadrul?) should also take a look.

(i wouldn't mind if a c/b/chromeos/events/OWNERS file were added at some
point...)

[sadrul, 2015-05-04 17:31:02]

https://codereview.chromium.org/1120813002/diff/120001/ui/events/event_consta...
File ui/events/event_constants.h (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/event_consta...
ui/events/event_constants.h:107: EF_FINAL              = 1 << 20,  // Do not
remap; the event was created with
Renumber these to be 16:18?

https://codereview.chromium.org/1120813002/diff/120001/ui/events/x/events_x_u...
File ui/events/x/events_x_unittest.cc (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/x/events_x_u...
ui/events/x/events_x_unittest.cc:50: #if !defined(OS_CHROMEOS)
?

[kpschoedel, 2015-05-04 17:49:19]

https://codereview.chromium.org/1120813002/diff/120001/ui/events/event_consta...
File ui/events/event_constants.h (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/event_consta...
ui/events/event_constants.h:107: EF_FINAL              = 1 << 20,  // Do not
remap; the event was created with
On 2015/05/04 17:31:01, sadrul wrote:
> Renumber these to be 16:18?

Done.

https://codereview.chromium.org/1120813002/diff/120001/ui/events/x/events_x_u...
File ui/events/x/events_x_unittest.cc (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/x/events_x_u...
ui/events/x/events_x_unittest.cc:50: #if !defined(OS_CHROMEOS)
On 2015/05/04 17:31:01, sadrul wrote:
> ?

With the removal of EventsXTest.NumpadKeyEvents this is only called under
OS_CHROMEOS, yielding an unused-function error.

[Wez, 2015-05-05 00:29:09]

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter.cc (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter.cc:146: } kLocations[] =
{{DomCode::CONTROL_LEFT, DomCodeLocation::LEFT},
Will this cause a static to be initialized on first-use, or just a pre-populated
text area with no initializer, since the content looks like POD?

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter.cc:182: {DomCode::NUMPAD_CLEAR_ENTRY,
DomCodeLocation::NUMPAD}};
nit: Although this is future-proof, you could reasonably restructure this as a
set of range-checks w/ some compile-time checks to protect against the enum
values getting messed-with, e.g. if (CONTROL_RIGHT <= code <= OS_RIGHT) return
::RIGHT...

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter.h (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter.h:19: enum class DomCodeLocation {
STANDARD, LEFT, RIGHT, NUMPAD };
On 2015/05/01 16:13:18, kpschoedel wrote:
> I know the spec calls these DOM_KEY_LOCATION_STANDARD, etc., but
DomKeyLocation
> would be confusing here because it's derived from a DomCode rather than a
> DomKey.

I'd argue that DomKeyLocation is the better name, since it is a location value
that identifies which specific instance a DomKey refers to, whereas for DomCode
the location information is implicit - to convey equivalent detail to the
DomCode, a call-site should handle both a DomKey and a DomKeyLocation.

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter.h:67: // Return the DomCodeLocation of
a DomCode.
Suggest stating explicitly that this is used to distinguish between instances of
keys with the same meaning, and therefore the same DomKey and/or VKEY.

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter_data.h (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter_data.h:229: USB_KEYMAP(0x070085,
0x0081, 0x0000, 0x005f, "NumpadComma", NUMPAD_COMMA),
On 2015/05/01 16:13:18, kpschoedel wrote:
> WebInputEventAuraTest uses this under X11.

Is that a standard code for that key under Linux systems? Do we have any
keyboards with this actual key? :D

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter_unittest.cc (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter_unittest.cc:21: const size_t
kExpectedMappedKeyCount = 168;
On 2015/05/01 16:13:18, kpschoedel wrote:
> I hate this test.

:P

IIRC I added it because I noticed I'd managed to break the tables for Linux in
one direction, and the mapped-key-count was an easy way to sanity-check it!

[kpschoedel, 2015-05-05 16:30:08]

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter.cc (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter.cc:146: } kLocations[] =
{{DomCode::CONTROL_LEFT, DomCodeLocation::LEFT},
On 2015/05/05 00:29:09, Wez wrote:
> Will this cause a static to be initialized on first-use, or just a
pre-populated
> text area with no initializer, since the content looks like POD?

'static const' lets it use the data directly from read-only text.

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter.cc:182: {DomCode::NUMPAD_CLEAR_ENTRY,
DomCodeLocation::NUMPAD}};
On 2015/05/05 00:29:09, Wez wrote:
> nit: Although this is future-proof, you could reasonably restructure this as a
> set of range-checks w/ some compile-time checks to protect against the enum
> values getting messed-with, e.g. if (CONTROL_RIGHT <= code <= OS_RIGHT) return
> ::RIGHT...

Yes; my reasoning was that the non-digit NUMPAD values are numerically
scattered, and they need to all be tested before we can return
DomCodeLocation::STANDARD. The most efficient code would probably come from
writing it as a switch and letting the compiler sort out the best way to express
the map.

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter.h (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter.h:19: enum class DomCodeLocation {
STANDARD, LEFT, RIGHT, NUMPAD };
On 2015/05/05 00:29:09, Wez wrote:
> On 2015/05/01 16:13:18, kpschoedel wrote:
> > I know the spec calls these DOM_KEY_LOCATION_STANDARD, etc., but
> DomKeyLocation
> > would be confusing here because it's derived from a DomCode rather than a
> > DomKey.
> 
> I'd argue that DomKeyLocation is the better name, since it is a location value
> that identifies which specific instance a DomKey refers to, whereas for
DomCode
> the location information is implicit - to convey equivalent detail to the
> DomCode, a call-site should handle both a DomKey and a DomKeyLocation.

Done.

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter.h:67: // Return the DomCodeLocation of
a DomCode.
On 2015/05/05 00:29:09, Wez wrote:
> Suggest stating explicitly that this is used to distinguish between instances
of
> keys with the same meaning, and therefore the same DomKey and/or VKEY.

Done.

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter_data.h (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter_data.h:229: USB_KEYMAP(0x070085,
0x0081, 0x0000, 0x005f, "NumpadComma", NUMPAD_COMMA),
On 2015/05/05 00:29:09, Wez wrote:
> On 2015/05/01 16:13:18, kpschoedel wrote:
> > WebInputEventAuraTest uses this under X11.
> 
> Is that a standard code for that key under Linux systems?

It's Linux KEY_KPCOMMA, which the kernel generates for USB 07:0085 (Keypad
Comma).

> Do we have any keyboards with this actual key? :D

The USB doc says Brazilian keyboards generate it, but I don't know whether
that's true in practice.

[Wez, 2015-05-05 21:23:45]

https://codereview.chromium.org/1120813002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/1120813002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/events/event_rewriter.cc:714: static const struct
NumPadRemapping {
On 2015/05/01 16:13:17, kpschoedel wrote:
> I'm in the process of revising this file to handle DomCode and DomKey
correctly,
> so this change will itself be replaced soon. Actually, since nothing in
> RewriteNumPadKeys depends on settings, I suspect it can be removed entirely in
> favour of generating the desired event in the first place (as already happens
> under Ozone).

Acknowledged.

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter.cc (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter.cc:182: {DomCode::NUMPAD_CLEAR_ENTRY,
DomCodeLocation::NUMPAD}};
On 2015/05/05 16:30:08, kpschoedel wrote:
> On 2015/05/05 00:29:09, Wez wrote:
> > nit: Although this is future-proof, you could reasonably restructure this as
a
> > set of range-checks w/ some compile-time checks to protect against the enum
> > values getting messed-with, e.g. if (CONTROL_RIGHT <= code <= OS_RIGHT)
return
> > ::RIGHT...
> 
> Yes; my reasoning was that the non-digit NUMPAD values are numerically
> scattered, and they need to all be tested before we can return
> DomCodeLocation::STANDARD. The most efficient code would probably come from
> writing it as a switch and letting the compiler sort out the best way to
express
> the map.

Acknowledged.

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter_data.h (right):

https://codereview.chromium.org/1120813002/diff/120001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter_data.h:229: USB_KEYMAP(0x070085,
0x0081, 0x0000, 0x005f, "NumpadComma", NUMPAD_COMMA),
On 2015/05/05 16:30:08, kpschoedel wrote:
> On 2015/05/05 00:29:09, Wez wrote:
> > On 2015/05/01 16:13:18, kpschoedel wrote:
> > > WebInputEventAuraTest uses this under X11.
> > 
> > Is that a standard code for that key under Linux systems?
> 
> It's Linux KEY_KPCOMMA, which the kernel generates for USB 07:0085 (Keypad
> Comma).
> 
> > Do we have any keyboards with this actual key? :D
> 
> The USB doc says Brazilian keyboards generate it, but I don't know whether
> that's true in practice.

OK, looks like Gary documented it at
http://www.w3.org/TR/DOM-Level-3-Events-code/#keyboard-104; will check w/ him
whether he has an example device.

https://codereview.chromium.org/1120813002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/1120813002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/events/event_rewriter.cc:292:
rewritten_key_event->native_event()->xkey.state |= Mod2Mask;
This code is super confusing; does native_event() refer to |numpad_event|, or to
|xkeyevent|, or to some separate internal XEvent...?

https://codereview.chromium.org/1120813002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/events/event_rewriter.cc:729: {ui::DomCode::NUMPAD9,
ui::VKEY_PRIOR, ui::VKEY_NUMPAD9},
Remind me - we always rewrite num-pad keys to behave as numeric keys, even if
the user has an external keyboard with a NumLock key?

https://codereview.chromium.org/1120813002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/events/event_rewriter.cc:731: for (const auto& map :
kNumPadRemappings) {
We end up with a lot of these remapping loops; if possible it'd be preferable to
merge the remappings into a single map so we can e.g. do a single binary search
in it to find remappings - in this case mapping
pair<DomCode,KeyboardCode>->KeyboardCode, although in future we're going to need
to handle DomKey as well.

https://codereview.chromium.org/1120813002/diff/160001/content/browser/render...
File content/browser/renderer_host/web_input_event_aura.cc (right):

https://codereview.chromium.org/1120813002/diff/160001/content/browser/render...
content/browser/renderer_host/web_input_event_aura.cc:65: default:
Can you use STANDARD here explicitly, rather than default, so we don't miss any
changes to the enum?

https://codereview.chromium.org/1120813002/diff/160001/content/browser/render...
File content/browser/renderer_host/web_input_event_aura_unittest.cc (right):

https://codereview.chromium.org/1120813002/diff/160001/content/browser/render...
content/browser/renderer_host/web_input_event_aura_unittest.cc:129: // Checks
that MakeWebKeyboardEvent fills a correct keypard modifier.
typo: keypard

https://codereview.chromium.org/1120813002/diff/160001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter_data.h (right):

https://codereview.chromium.org/1120813002/diff/160001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter_data.h:228: // USB#070085 is used as
Brazilian Keypad_.
Looks like this comment can be removed - doesn't seem to add anything?

[Wez, 2015-05-05 21:24:12]

Modulo the nits above, LGTM :)

[kpschoedel, 2015-05-05 22:19:08]

https://codereview.chromium.org/1120813002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/1120813002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/events/event_rewriter.cc:292:
rewritten_key_event->native_event()->xkey.state |= Mod2Mask;
On 2015/05/05 21:23:44, Wez wrote:
> This code is super confusing;

It is (and I don't actually understand the intent of this block). Fortunately
the whole USE_X11 section should be removable as soon as EventRewriter starts
rewriting DomCode & DomKey correctly. (That's next on my list.)

> does native_event() refer to |numpad_event|, or to
> |xkeyevent|, or to some separate internal XEvent...?

It's a copy of xkeyevent owned by rewritten_key_event; i.e. the modified event
that EventRewriter returns. Mod2Mask needs to be set because ChromeOS always
interprets numpad keys as numbers. Really the lookup should handle this and we
shouldn't have to set it explicitly on the event, but that X11 lookup code is a
bit fragmented (and further down my list).

https://codereview.chromium.org/1120813002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/events/event_rewriter.cc:729: {ui::DomCode::NUMPAD9,
ui::VKEY_PRIOR, ui::VKEY_NUMPAD9},
On 2015/05/05 21:23:44, Wez wrote:
> Remind me - we always rewrite num-pad keys to behave as numeric keys, even if
> the user has an external keyboard with a NumLock key?

Yes. In the future, either when/if I get around to refactoring X11 lookup, or
when no ChromeOS still use X11, this function will go away, because we'll
generate the correct event to start with.

https://codereview.chromium.org/1120813002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/events/event_rewriter.cc:731: for (const auto& map :
kNumPadRemappings) {
On 2015/05/05 21:23:44, Wez wrote:
> We end up with a lot of these remapping loops; if possible it'd be preferable
to
> merge the remappings into a single map so we can e.g. do a single binary
search
> in it to find remappings - in this case mapping
> pair<DomCode,KeyboardCode>->KeyboardCode, although in future we're going to
need
> to handle DomKey as well.

Yes, I've been considering this as I do the work to handle DomCode & DomKey,
though I'm currently leaning toward doing that expediently in hope of getting to
the point where we can remove this entirely.

https://codereview.chromium.org/1120813002/diff/160001/content/browser/render...
File content/browser/renderer_host/web_input_event_aura.cc (right):

https://codereview.chromium.org/1120813002/diff/160001/content/browser/render...
content/browser/renderer_host/web_input_event_aura.cc:65: default:
On 2015/05/05 21:23:44, Wez wrote:
> Can you use STANDARD here explicitly, rather than default, so we don't miss
any
> changes to the enum?

Done.

https://codereview.chromium.org/1120813002/diff/160001/content/browser/render...
File content/browser/renderer_host/web_input_event_aura_unittest.cc (right):

https://codereview.chromium.org/1120813002/diff/160001/content/browser/render...
content/browser/renderer_host/web_input_event_aura_unittest.cc:129: // Checks
that MakeWebKeyboardEvent fills a correct keypard modifier.
On 2015/05/05 21:23:44, Wez wrote:
> typo: keypard

Done.

https://codereview.chromium.org/1120813002/diff/160001/ui/events/keycodes/dom...
File ui/events/keycodes/dom4/keycode_converter_data.h (right):

https://codereview.chromium.org/1120813002/diff/160001/ui/events/keycodes/dom...
ui/events/keycodes/dom4/keycode_converter_data.h:228: // USB#070085 is used as
Brazilian Keypad_.
On 2015/05/05 21:23:44, Wez wrote:
> Looks like this comment can be removed - doesn't seem to add anything?

Done.

[sadrul, 2015-05-06 03:17:22]

lgtm

[kpschoedel, 2015-05-06 14:08:16]

The CQ bit was checked by kpschoedel@chromium.org

[kpschoedel, 2015-05-06 14:08:16]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org,
wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1120813002/#ps180001
(title: "address review comments (wez@)")

[commit-bot: I haz the power, 2015-05-06 14:08:40]

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

[commit-bot: I haz the power, 2015-05-06 15:49:13]

Committed patchset #5 (id:180001)

[commit-bot: I haz the power, 2015-05-06 15:50:10]

Patchset 5 (id:??) landed as
https://crrev.com/8226fd15e53b122f531b3b1816b3e080bd8ef011
Cr-Commit-Position: refs/heads/master@{#328543}

[Nico, 2015-05-09 20:47:52]

This made WebInputEventAuraTest.TestMakeWebKeyboardEventKeyPadKeyCode reliably
fail on the chromeos valgrind bot:

http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v...

WebInputEventAuraTest.TestMakeWebKeyboardEventKeyPadKeyCode:
../../content/browser/renderer_host/web_input_event_aura_unittest.cc:205:
Failure
Value of: (webkit_event.modifiers & blink::WebInputEvent::IsKeyPad) != 0
Actual: false
Expected: test_case.expected_result
Which is: true
Failed in 23th test case: {dom_code:NumpadDivide, ui_keycode:111,
x_keysym:65455}, expect: true
../../content/browser/renderer_host/web_input_event_aura_unittest.cc:205:
Failure
Value of: (webkit_event.modifiers & blink::WebInputEvent::IsKeyPad) != 0
Actual: false
Expected: test_case.expected_result
Which is: true
Failed in 24th test case: {dom_code:NumpadDecimal, ui_keycode:110,
x_keysym:65454}, expect: true

Do you see an obvious fix?

[kpschoedel, 2015-05-10 17:07:29]

On 2015/05/09 20:47:52, Nico wrote:
> This made
TestMakeWebKeyboardEventKeyPadKeyCode.TestMakeWebKeyboardEventKeyPadKeyCode
reliably
> fail on the chromeos valgrind bot:
> 
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v...
> 
> WebInputEventAuraTest.TestMakeWebKeyboardEventKeyPadKeyCode:
> ../../content/browser/renderer_host/web_input_event_aura_unittest.cc:205:
> Failure
> Value of: (webkit_event.modifiers & blink::WebInputEvent::IsKeyPad) != 0
> Actual: false
> Expected: test_case.expected_result
> Which is: true
> Failed in 23th test case: {dom_code:NumpadDivide, ui_keycode:111,
> x_keysym:65455}, expect: true
> ../../content/browser/renderer_host/web_input_event_aura_unittest.cc:205:
> Failure
> Value of: (webkit_event.modifiers & blink::WebInputEvent::IsKeyPad) != 0
> Actual: false
> Expected: test_case.expected_result
> Which is: true
> Failed in 24th test case: {dom_code:NumpadDecimal, ui_keycode:110,
> x_keysym:65454}, expect: true
> 
> Do you see an obvious fix?

This should be fixed by crrev.com/a1e51ea7000ea7da6c477ea6ec0cc93a724297d4

[benwells, 2015-05-12 00:59:23]

On 2015/05/10 17:07:29, kpschoedel wrote:
> On 2015/05/09 20:47:52, Nico wrote:
> > This made
> TestMakeWebKeyboardEventKeyPadKeyCode.TestMakeWebKeyboardEventKeyPadKeyCode
> reliably
> > fail on the chromeos valgrind bot:
> > 
> >
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v...
> > 
> > WebInputEventAuraTest.TestMakeWebKeyboardEventKeyPadKeyCode:
> > ../../content/browser/renderer_host/web_input_event_aura_unittest.cc:205:
> > Failure
> > Value of: (webkit_event.modifiers & blink::WebInputEvent::IsKeyPad) != 0
> > Actual: false
> > Expected: test_case.expected_result
> > Which is: true
> > Failed in 23th test case: {dom_code:NumpadDivide, ui_keycode:111,
> > x_keysym:65455}, expect: true
> > ../../content/browser/renderer_host/web_input_event_aura_unittest.cc:205:
> > Failure
> > Value of: (webkit_event.modifiers & blink::WebInputEvent::IsKeyPad) != 0
> > Actual: false
> > Expected: test_case.expected_result
> > Which is: true
> > Failed in 24th test case: {dom_code:NumpadDecimal, ui_keycode:110,
> > x_keysym:65454}, expect: true
> > 
> > Do you see an obvious fix?
> 
> This should be fixed by crrev.com/a1e51ea7000ea7da6c477ea6ec0cc93a724297d4

This is still failing on the ChromeOS Valgrind bot. I will revert soon.

[benwells, 2015-05-12 01:30:06]

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

The reason for reverting is: This is failing on the chromeos valgrind bot. e.g.:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v...

Sample output:
WebInputEventAuraTest.TestMakeWebKeyboardEventKeyPadKeyCode:
../../content/browser/renderer_host/web_input_event_aura_unittest.cc:207:
Failure
Value of: (webkit_event.modifiers & blink::WebInputEvent::IsKeyPad) != 0
Actual: false
Expected: test_case.expected_result
Which is: true
Failed in 23th test case: {dom_code:NumpadDivide, ui_keycode:111,
x_keysym:65455}, expect: true
../../content/browser/renderer_host/web_input_event_aura_unittest.cc:207:
Failure
Value of: (webkit_event.modifiers & blink::WebInputEvent::IsKeyPad) != 0
Actual: false
Expected: test_case.expected_result
Which is: true
Failed in 24th test case: {dom_code:NumpadDecimal, ui_keycode:110,
x_keysym:65454}, expect: true.

[benwells, 2015-05-12 03:00:01]

On 2015/05/12 01:30:06, benwells wrote:
> A revert of this CL (patchset #5 id:180001) has been created in
> https://codereview.chromium.org/1135083004/ by mailto:benwells@chromium.org.
> 
> The reason for reverting is: This is failing on the chromeos valgrind bot.
e.g.:
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v...
> 
> Sample output:
> WebInputEventAuraTest.TestMakeWebKeyboardEventKeyPadKeyCode:
> ../../content/browser/renderer_host/web_input_event_aura_unittest.cc:207:
> Failure
> Value of: (webkit_event.modifiers & blink::WebInputEvent::IsKeyPad) != 0
> Actual: false
> Expected: test_case.expected_result
> Which is: true
> Failed in 23th test case: {dom_code:NumpadDivide, ui_keycode:111,
> x_keysym:65455}, expect: true
> ../../content/browser/renderer_host/web_input_event_aura_unittest.cc:207:
> Failure
> Value of: (webkit_event.modifiers & blink::WebInputEvent::IsKeyPad) != 0
> Actual: false
> Expected: test_case.expected_result
> Which is: true
> Failed in 24th test case: {dom_code:NumpadDecimal, ui_keycode:110,
> x_keysym:65454}, expect: true.

This revert conflicted with another change. Instead of reverting I'm going to
disable the test. Please investigate and re-enable ASAP.