Clean up event flags a bit:

Clean up event flags a bit:

* Reorder to group related items together.  Add group comments.
* Rename {NUM,CAPS,SCROLL}_LOCK_DOWN to ..._ON, in hopes of avoiding confusion
  as to whether the event is about one of these keys actually being pressed down
  versus just conveying the current state of the various locks.
* Rename EF_EXTENDED to EF_IS_EXTENDED_KEY in hopes of greater clarity.  Move it
  to the KeyEvent-specific enum since it's non-sensical for non-KeyEvents.
* Expose MouseEvent::button_flags() since a couple callers outside the class
  wanted it.
* Move Event::IsRepeat() to KeyEvent (to match the enum placement) and rename it
  unix_hacker()-style.  (There are a ton of other functions in event.h that
  should be named that way, but there's only so much drudgery I can stand.)
* Add some missing bits: missing static_asserts in the Mojo code that didn't
  check all enum values were equal, a missing SCROLL_LOCK_ON check in the exo
  code that would have no functional effect but looked unduly suspicious.
* Attempt to reorder code to either check/use these enum values in the same
  order they're declared, or, in cases like code mapping GTK enum values to
  these values, in the order of the GTK enum values (and similar).

BUG=none
TEST=none

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

[Peter Kasting, 2016-01-05 03:23:17]

Description was changed from

==========
Clean up event flags a bit:

* Reorder to group related items together.  Add group comments.
* Rename {NUM,CAPS,SCROLL}_LOCK_DOWN to ..._ON, in hopes of avoiding confusion
  as to whether the event is about one of these keys actually being pressed down
  versus just conveying the current state of the various locks.
* Expose MouseEvent::button_flags() since a couple callers outside the class
  wanted it.
* Add some missing bits: missing static_asserts in the Mojo code that didn't
  check all enum values were equal, a missing SCROLL_LOCK_ON check in the exo
  code that would have no functional effect but looked unduly suspicious.
* Attempt to reorder code to either check/use these enum values in the same
  order they're declared, or, in cases like code mapping GTK enum values to
  these values, in the order of the GTK enum values (and similar).

BUG=none
TEST=none
==========

to

==========
Clean up event flags a bit:

* Reorder to group related items together.  Add group comments.
* Rename {NUM,CAPS,SCROLL}_LOCK_DOWN to ..._ON, in hopes of avoiding confusion
  as to whether the event is about one of these keys actually being pressed down
  versus just conveying the current state of the various locks.
* Rename EF_EXTENDED to EF_IS_EXTENDED_KEY in hopes of greater clarity.  Move it
  to the KeyEvent-specific enum since it's non-sensical for non-KeyEvents.
* Expose MouseEvent::button_flags() since a couple callers outside the class
  wanted it.
* Move Event::IsRepeat() to KeyEvent (to match the enum placement) and rename it
  unix_hacker()-style.  (There are a ton of other functions in event.h that
  should be named that way, but there's only so much drudgery I can stand.)
* Add some missing bits: missing static_asserts in the Mojo code that didn't
  check all enum values were equal, a missing SCROLL_LOCK_ON check in the exo
  code that would have no functional effect but looked unduly suspicious.
* Attempt to reorder code to either check/use these enum values in the same
  order they're declared, or, in cases like code mapping GTK enum values to
  these values, in the order of the GTK enum values (and similar).

BUG=none
TEST=none
==========

[Peter Kasting, 2016-01-05 03:29:23]

pkasting@chromium.org changed reviewers:
+ sadrul@chromium.org

[Peter Kasting, 2016-01-05 03:29:24]

https://codereview.chromium.org/1559163002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/1559163002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter_unittest.cc:961:
EXPECT_EQ(GetExpectedResultAsString(
Note: The formatting changes in this file and
input_method_engine_browsertests.cc are because presubmit demanded I run git cl
format, which did this.

[sadrul, 2016-01-06 17:42:12]

lgtm

https://codereview.chromium.org/1559163002/diff/40001/ui/content_accelerators...
File ui/content_accelerators/accelerator_util.cc (right):

https://codereview.chromium.org/1559163002/diff/40001/ui/content_accelerators...
ui/content_accelerators/accelerator_util.cc:42: if (event.os_event &&
event.os_event->IsKeyEvent() &&
It shouldn't be necessary to check IsKeyEvent() here, since we are getting the
event from a NativeWebKeyboardEvent here. Perhaps DCHECK(!event.os_event ||
event.os_event->IsKeyEvent()) instead?

[Peter Kasting, 2016-01-06 20:24:02]

pkasting@chromium.org changed reviewers:
+ reveman@chromium.org, shuchen@chromium.org, sky@chromium.org

[Peter Kasting, 2016-01-06 20:25:15]

Adding OWNERS:

sky: ash/, components/mus/, mojo/
shuchen: chrome/browser/chromeos/input_method/
reveman: components/exo/

https://codereview.chromium.org/1559163002/diff/40001/ui/content_accelerators...
File ui/content_accelerators/accelerator_util.cc (right):

https://codereview.chromium.org/1559163002/diff/40001/ui/content_accelerators...
ui/content_accelerators/accelerator_util.cc:42: if (event.os_event &&
event.os_event->IsKeyEvent() &&
On 2016/01/06 17:42:12, sadrul wrote:
> It shouldn't be necessary to check IsKeyEvent() here, since we are getting the
> event from a NativeWebKeyboardEvent here. Perhaps DCHECK(!event.os_event ||
> event.os_event->IsKeyEvent()) instead?

I'll just remove the IsKeyEvent() check.

[Shu Chen, 2016-01-06 20:38:49]

lgtm for chrome/browser/chromeos/input_method/...

[sky, 2016-01-06 21:56:37]

LGTM

[reveman, 2016-01-07 12:02:59]

lgtm with nit

https://codereview.chromium.org/1559163002/diff/60001/components/exo/keyboard.cc
File components/exo/keyboard.cc (right):

https://codereview.chromium.org/1559163002/diff/60001/components/exo/keyboard...
components/exo/keyboard.cc:42: ui::EF_NUM_LOCK_ON | ui::EF_CAPS_LOCK_ON |
ui::EF_SCROLL_LOCK_ON;
nit: please avoid adding ui::EF_SCROLL_LOCK_ON as part of this patch as it
requires additional changes to the wayland code to avoid sending unnecessary
modifier events. ie. if scroll lock state changes then we'll send modifier
events to wayland clients without any modifiers that the client is aware of
actually changing.

[Peter Kasting, 2016-01-07 12:15:24]

https://codereview.chromium.org/1559163002/diff/60001/components/exo/keyboard.cc
File components/exo/keyboard.cc (right):

https://codereview.chromium.org/1559163002/diff/60001/components/exo/keyboard...
components/exo/keyboard.cc:42: ui::EF_NUM_LOCK_ON | ui::EF_CAPS_LOCK_ON |
ui::EF_SCROLL_LOCK_ON;
On 2016/01/07 12:02:59, reveman wrote:
> nit: please avoid adding ui::EF_SCROLL_LOCK_ON as part of this patch as it
> requires additional changes to the wayland code to avoid sending unnecessary
> modifier events. ie. if scroll lock state changes then we'll send modifier
> events to wayland clients without any modifiers that the client is aware of
> actually changing.

Oh, I thought you had told me this would have no effect.  Sure, I'll remove it
from this change then.

Do you want me to write a separate change for this, or do something else?

[reveman, 2016-01-07 12:32:37]

https://codereview.chromium.org/1559163002/diff/60001/components/exo/keyboard.cc
File components/exo/keyboard.cc (right):

https://codereview.chromium.org/1559163002/diff/60001/components/exo/keyboard...
components/exo/keyboard.cc:42: ui::EF_NUM_LOCK_ON | ui::EF_CAPS_LOCK_ON |
ui::EF_SCROLL_LOCK_ON;
On 2016/01/07 at 12:15:24, Peter Kasting wrote:
> On 2016/01/07 12:02:59, reveman wrote:
> > nit: please avoid adding ui::EF_SCROLL_LOCK_ON as part of this patch as it
> > requires additional changes to the wayland code to avoid sending unnecessary
> > modifier events. ie. if scroll lock state changes then we'll send modifier
> > events to wayland clients without any modifiers that the client is aware of
> > actually changing.
> 
> Oh, I thought you had told me this would have no effect.  Sure, I'll remove it
from this change then.

Sorry, I failed to think of this before. It wouldn't break anything but it would
have a subtle change in behavior which I think is best to avoid as part of this
patch.

> 
> Do you want me to write a separate change for this, or do something else?

It would be more correct to move this into the wayland code (ie. move this
modifider mask to the KeyboardDelegate) but I think we can just leave this as is
for now.

[Peter Kasting, 2016-01-07 13:15:54]

On 2016/01/07 12:32:37, reveman wrote:
> On 2016/01/07 at 12:15:24, Peter Kasting wrote:
> > Do you want me to write a separate change for this, or do something else?
> 
> It would be more correct to move this into the wayland code (ie. move this
> modifider mask to the KeyboardDelegate) but I think we can just leave this as
is
> for now.

OK... I'm not terribly keen on leaving that out indefinitely, especially as it's
the change which caused me to do the rest of this to begin with; but I will
leave it to you to take care of.

[reveman, 2016-01-08 12:01:53]

On 2016/01/07 at 13:15:54, pkasting wrote:
> On 2016/01/07 12:32:37, reveman wrote:
> > On 2016/01/07 at 12:15:24, Peter Kasting wrote:
> > > Do you want me to write a separate change for this, or do something else?
> > 
> > It would be more correct to move this into the wayland code (ie. move this
> > modifider mask to the KeyboardDelegate) but I think we can just leave this
as is
> > for now.
> 
> OK... I'm not terribly keen on leaving that out indefinitely, especially as
it's the change which caused me to do the rest of this to begin with; but I will
leave it to you to take care of.

I think I prefer to leave it as is for now. This modifier mask is basically the
set of modifiers that exo clients should be aware of. Removing some of the
modifiers from this mask is not a problem, it just means exo clients won't see
them. Wayland protocol doesn't support scroll lock so that needs to be removed
somewhere. If we eventually have some other exo bindings that support scroll
lock, then I'll change this so that exo supports all modifiers that chrome
supports and wayland removes scroll lock. For now I'm thinking we can just add a
comment next to the modifier mask to explain why scroll lock is not included.

[Peter Kasting, 2016-01-08 21:26:07]

On 2016/01/08 12:01:53, reveman wrote:
> On 2016/01/07 at 13:15:54, pkasting wrote:
> > On 2016/01/07 12:32:37, reveman wrote:
> > > On 2016/01/07 at 12:15:24, Peter Kasting wrote:
> > > > Do you want me to write a separate change for this, or do something
else?
> > > 
> > > It would be more correct to move this into the wayland code (ie. move this
> > > modifider mask to the KeyboardDelegate) but I think we can just leave this
> as is
> > > for now.
> > 
> > OK... I'm not terribly keen on leaving that out indefinitely, especially as
> it's the change which caused me to do the rest of this to begin with; but I
will
> leave it to you to take care of.
> 
> I think I prefer to leave it as is for now. This modifier mask is basically
the
> set of modifiers that exo clients should be aware of. Removing some of the
> modifiers from this mask is not a problem, it just means exo clients won't see
> them. Wayland protocol doesn't support scroll lock so that needs to be removed
> somewhere. If we eventually have some other exo bindings that support scroll
> lock, then I'll change this so that exo supports all modifiers that chrome
> supports and wayland removes scroll lock. For now I'm thinking we can just add
a
> comment next to the modifier mask to explain why scroll lock is not included.

OK.  Removed the scroll lock modifier and added such a comment.  I'm going to go
ahead and check in, if I misstated the comment in any way feel free to direct me
to make a followup change.

[Peter Kasting, 2016-01-08 21:26:23]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-01-08 21:26:23]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
reveman@chromium.org, shuchen@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1559163002/#ps80001
(title: "Review comment")

[commit-bot: I haz the power, 2016-01-08 21:26:59]

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

[reveman, 2016-01-08 21:31:13]

lgtm

[commit-bot: I haz the power, 2016-01-08 23:38:56]

Description was changed from

==========
Clean up event flags a bit:

* Reorder to group related items together.  Add group comments.
* Rename {NUM,CAPS,SCROLL}_LOCK_DOWN to ..._ON, in hopes of avoiding confusion
  as to whether the event is about one of these keys actually being pressed down
  versus just conveying the current state of the various locks.
* Rename EF_EXTENDED to EF_IS_EXTENDED_KEY in hopes of greater clarity.  Move it
  to the KeyEvent-specific enum since it's non-sensical for non-KeyEvents.
* Expose MouseEvent::button_flags() since a couple callers outside the class
  wanted it.
* Move Event::IsRepeat() to KeyEvent (to match the enum placement) and rename it
  unix_hacker()-style.  (There are a ton of other functions in event.h that
  should be named that way, but there's only so much drudgery I can stand.)
* Add some missing bits: missing static_asserts in the Mojo code that didn't
  check all enum values were equal, a missing SCROLL_LOCK_ON check in the exo
  code that would have no functional effect but looked unduly suspicious.
* Attempt to reorder code to either check/use these enum values in the same
  order they're declared, or, in cases like code mapping GTK enum values to
  these values, in the order of the GTK enum values (and similar).

BUG=none
TEST=none
==========

to

==========
Clean up event flags a bit:

* Reorder to group related items together.  Add group comments.
* Rename {NUM,CAPS,SCROLL}_LOCK_DOWN to ..._ON, in hopes of avoiding confusion
  as to whether the event is about one of these keys actually being pressed down
  versus just conveying the current state of the various locks.
* Rename EF_EXTENDED to EF_IS_EXTENDED_KEY in hopes of greater clarity.  Move it
  to the KeyEvent-specific enum since it's non-sensical for non-KeyEvents.
* Expose MouseEvent::button_flags() since a couple callers outside the class
  wanted it.
* Move Event::IsRepeat() to KeyEvent (to match the enum placement) and rename it
  unix_hacker()-style.  (There are a ton of other functions in event.h that
  should be named that way, but there's only so much drudgery I can stand.)
* Add some missing bits: missing static_asserts in the Mojo code that didn't
  check all enum values were equal, a missing SCROLL_LOCK_ON check in the exo
  code that would have no functional effect but looked unduly suspicious.
* Attempt to reorder code to either check/use these enum values in the same
  order they're declared, or, in cases like code mapping GTK enum values to
  these values, in the order of the GTK enum values (and similar).

BUG=none
TEST=none
==========

[commit-bot: I haz the power, 2016-01-08 23:38:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-08 23:40:29]

Description was changed from

==========
Clean up event flags a bit:

* Reorder to group related items together.  Add group comments.
* Rename {NUM,CAPS,SCROLL}_LOCK_DOWN to ..._ON, in hopes of avoiding confusion
  as to whether the event is about one of these keys actually being pressed down
  versus just conveying the current state of the various locks.
* Rename EF_EXTENDED to EF_IS_EXTENDED_KEY in hopes of greater clarity.  Move it
  to the KeyEvent-specific enum since it's non-sensical for non-KeyEvents.
* Expose MouseEvent::button_flags() since a couple callers outside the class
  wanted it.
* Move Event::IsRepeat() to KeyEvent (to match the enum placement) and rename it
  unix_hacker()-style.  (There are a ton of other functions in event.h that
  should be named that way, but there's only so much drudgery I can stand.)
* Add some missing bits: missing static_asserts in the Mojo code that didn't
  check all enum values were equal, a missing SCROLL_LOCK_ON check in the exo
  code that would have no functional effect but looked unduly suspicious.
* Attempt to reorder code to either check/use these enum values in the same
  order they're declared, or, in cases like code mapping GTK enum values to
  these values, in the order of the GTK enum values (and similar).

BUG=none
TEST=none
==========

to

==========
Clean up event flags a bit:

* Reorder to group related items together.  Add group comments.
* Rename {NUM,CAPS,SCROLL}_LOCK_DOWN to ..._ON, in hopes of avoiding confusion
  as to whether the event is about one of these keys actually being pressed down
  versus just conveying the current state of the various locks.
* Rename EF_EXTENDED to EF_IS_EXTENDED_KEY in hopes of greater clarity.  Move it
  to the KeyEvent-specific enum since it's non-sensical for non-KeyEvents.
* Expose MouseEvent::button_flags() since a couple callers outside the class
  wanted it.
* Move Event::IsRepeat() to KeyEvent (to match the enum placement) and rename it
  unix_hacker()-style.  (There are a ton of other functions in event.h that
  should be named that way, but there's only so much drudgery I can stand.)
* Add some missing bits: missing static_asserts in the Mojo code that didn't
  check all enum values were equal, a missing SCROLL_LOCK_ON check in the exo
  code that would have no functional effect but looked unduly suspicious.
* Attempt to reorder code to either check/use these enum values in the same
  order they're declared, or, in cases like code mapping GTK enum values to
  these values, in the order of the GTK enum values (and similar).

BUG=none
TEST=none

Committed: https://crrev.com/cc7f6acc589a1be4c65a9562f77596278f62d561
Cr-Commit-Position: refs/heads/master@{#368456}
==========

[commit-bot: I haz the power, 2016-01-08 23:40:30]

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