Ignore inserting character with Search as a modifier on ChromeOS

Ignore inserting character with Search as a modifier on ChromeOS

We want to treat the Search key on ChromeOS as a modifier like Alt and
Ctrl, so that pressing Search+<key> doesn't result in a character in a
textfield or WebContents.

R=oshima@chromium.org
BUG=471488
TEST=views_unittests --gtest_filter=TextfieldTest.KeysWithModifiersTest

Committed: https://crrev.com/8518ee7bce034068a4c9a398e3865d0327fb14ee
Cr-Commit-Position: refs/heads/master@{#347514}

[afakhry, 2015-09-04 16:39:47]

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

[afakhry, 2015-09-04 16:43:36]

Here's an alternative solution to the one presented in the former CL there:
https://codereview.chromium.org/1221083008/.

In this CL, we treat the Search key on ChromeOS the same way as Alt is treated,
as a system key, so the insertion of the character is skipped. See
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Please take a look.

[oshima, 2015-09-04 17:05:28]

this looks reasonable to me. kpschoedel@, what do you think?

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

https://codereview.chromium.org/1325333002/diff/1/ui/events/base_event_utils....
ui/events/base_event_utils.cc:40: return (kModifierMask & flags) != 0;
return (allow_altgr || (flags & EF_ALTGR_DOWN) == 0) &&
       (kModifier & flags) != 0;

?

https://codereview.chromium.org/1325333002/diff/1/ui/events/base_event_utils.h
File ui/events/base_event_utils.h (right):

https://codereview.chromium.org/1325333002/diff/1/ui/events/base_event_utils....
ui/events/base_event_utils.h:17: EVENTS_BASE_EXPORT bool IsSystemKey(int flags,
bool allow_altgr);
IsSystemKeyModifier ?

[afakhry, 2015-09-04 17:26:03]

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

https://codereview.chromium.org/1325333002/diff/1/ui/events/base_event_utils....
ui/events/base_event_utils.cc:40: return (kModifierMask & flags) != 0;
On 2015/09/04 17:05:28, oshima wrote:
> return (allow_altgr || (flags & EF_ALTGR_DOWN) == 0) &&
>        (kModifier & flags) != 0;
> 
> ?

Done.

https://codereview.chromium.org/1325333002/diff/1/ui/events/base_event_utils.h
File ui/events/base_event_utils.h (right):

https://codereview.chromium.org/1325333002/diff/1/ui/events/base_event_utils....
ui/events/base_event_utils.h:17: EVENTS_BASE_EXPORT bool IsSystemKey(int flags,
bool allow_altgr);
On 2015/09/04 17:05:28, oshima wrote:
> IsSystemKeyModifier ?

Done.

[kpschoedel, 2015-09-04 18:06:13]

Looks fine, with comments.

(I don't own any of these files.)

https://codereview.chromium.org/1325333002/diff/20001/ui/events/base_event_ut...
File ui/events/base_event_utils.cc (right):

https://codereview.chromium.org/1325333002/diff/20001/ui/events/base_event_ut...
ui/events/base_event_utils.cc:16: const int kModifierMask = EF_ALT_DOWN |
EF_COMMAND_DOWN;
nit: kSystemKeyModifierMask for consistency

https://codereview.chromium.org/1325333002/diff/20001/ui/events/base_event_ut...
File ui/events/base_event_utils.h (right):

https://codereview.chromium.org/1325333002/diff/20001/ui/events/base_event_ut...
ui/events/base_event_utils.h:20: EVENTS_BASE_EXPORT bool IsSystemKeyModifier(int
flags, bool allow_altgr);
I think the ‘allow_altgr’ parameter makes this harder to follow at the call
sites, and would prefer just |IsSystemKeyModifier(flags)| with the separate
AltGr test retained in the one place that needs it.

[afakhry, 2015-09-04 18:26:36]

https://codereview.chromium.org/1325333002/diff/20001/ui/events/base_event_ut...
File ui/events/base_event_utils.cc (right):

https://codereview.chromium.org/1325333002/diff/20001/ui/events/base_event_ut...
ui/events/base_event_utils.cc:16: const int kModifierMask = EF_ALT_DOWN |
EF_COMMAND_DOWN;
On 2015/09/04 18:06:13, kpschoedel wrote:
> nit: kSystemKeyModifierMask for consistency

Done.

https://codereview.chromium.org/1325333002/diff/20001/ui/events/base_event_ut...
File ui/events/base_event_utils.h (right):

https://codereview.chromium.org/1325333002/diff/20001/ui/events/base_event_ut...
ui/events/base_event_utils.h:20: EVENTS_BASE_EXPORT bool IsSystemKeyModifier(int
flags, bool allow_altgr);
On 2015/09/04 18:06:13, kpschoedel wrote:
> I think the ‘allow_altgr’ parameter makes this harder to follow at the call
> sites, and would prefer just |IsSystemKeyModifier(flags)| with the separate
> AltGr test retained in the one place that needs it.

I agree. Done.

[afakhry, 2015-09-04 18:51:48]

afakhry@chromium.org changed reviewers:
+ avi@chromium.org

[afakhry, 2015-09-04 18:51:48]

avi@chromium.org: Could you please review these files:

content/browser/renderer_host/native_web_keyboard_event_aura.cc
ui/events/base_event_utils.h
ui/events/base_event_utils.cc
?
Thanks!

[Avi (use Gerrit), 2015-09-04 20:03:16]

LGTM

[oshima, 2015-09-04 21:48:32]

lgtm

[afakhry, 2015-09-04 21:49:59]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2015-09-04 21:50:37]

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

[commit-bot: I haz the power, 2015-09-04 22:50:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-04 22:50:46]

Patchset 3 (id:??) landed as
https://crrev.com/8518ee7bce034068a4c9a398e3865d0327fb14ee
Cr-Commit-Position: refs/heads/master@{#347514}