Fix sticky keys crash when handling synthetic events without a native event.

Fix sticky keys crash when handling synthetic events without a native event.

BUG=358270
TEST=modified tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262465

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262529

[Tim Song, 2014-04-07 19:48:27]

Please take a look, and let me know if there is a better way to do this.

[sadrul, 2014-04-07 21:57:33]

https://codereview.chromium.org/227113009/diff/20001/ash/sticky_keys/sticky_k...
File ash/sticky_keys/sticky_keys_controller.cc (right):

https://codereview.chromium.org/227113009/diff/20001/ash/sticky_keys/sticky_k...
ash/sticky_keys/sticky_keys_controller.cc:81: event->flags());
It looks like this ctor for MouseWheelEvent is not used anywhere. Mind changing
the ctor so it doesn't take the EventType or the flags as parameters?

https://codereview.chromium.org/227113009/diff/20001/ash/sticky_keys/sticky_k...
ash/sticky_keys/sticky_keys_controller.cc:96: }
You can always just create the new event out of the modified event and dispatch
that, right? Does creating the event from the X event give you any benefit?

[Tim Song, 2014-04-07 22:23:27]

https://codereview.chromium.org/227113009/diff/20001/ash/sticky_keys/sticky_k...
File ash/sticky_keys/sticky_keys_controller.cc (right):

https://codereview.chromium.org/227113009/diff/20001/ash/sticky_keys/sticky_k...
ash/sticky_keys/sticky_keys_controller.cc:81: event->flags());
On 2014/04/07 21:57:33, sadrul wrote:
> It looks like this ctor for MouseWheelEvent is not used anywhere. Mind
changing
> the ctor so it doesn't take the EventType or the flags as parameters?

Done.

https://codereview.chromium.org/227113009/diff/20001/ash/sticky_keys/sticky_k...
ash/sticky_keys/sticky_keys_controller.cc:96: }
On 2014/04/07 21:57:33, sadrul wrote:
> You can always just create the new event out of the modified event and
dispatch
> that, right? Does creating the event from the X event give you any benefit?

Done. You're right, it should be equivalent in this case.

[sadrul, 2014-04-07 22:32:10]

https://codereview.chromium.org/227113009/diff/40001/ash/sticky_keys/sticky_k...
File ash/sticky_keys/sticky_keys_controller.cc (right):

https://codereview.chromium.org/227113009/diff/40001/ash/sticky_keys/sticky_k...
ash/sticky_keys/sticky_keys_controller.cc:491: XEvent* xev =
event->native_event();
Since you are creating the mouse-event directly from the ui::MouseEvent, do you
need to mutate the X event anymore?

https://codereview.chromium.org/227113009/diff/40001/ash/sticky_keys/sticky_k...
ash/sticky_keys/sticky_keys_controller.cc:516: event->set_flags(event->flags() |
modifier_flag_);
Since this is not the event you dispatch, do you need to change the flags here?

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

https://codereview.chromium.org/227113009/diff/40001/ui/events/event.h#newcod...
ui/events/event.h:420: : MouseEvent(model, source, target, model.type(),
model.flags()),
You don't need the last two params. There's a MouseEvent(model, source, target)
ctor that should be sufficient.

[Tim Song, 2014-04-07 22:53:59]

https://codereview.chromium.org/227113009/diff/40001/ash/sticky_keys/sticky_k...
File ash/sticky_keys/sticky_keys_controller.cc (right):

https://codereview.chromium.org/227113009/diff/40001/ash/sticky_keys/sticky_k...
ash/sticky_keys/sticky_keys_controller.cc:491: XEvent* xev =
event->native_event();
On 2014/04/07 22:32:10, sadrul wrote:
> Since you are creating the mouse-event directly from the ui::MouseEvent, do
you
> need to mutate the X event anymore?

The X event modification was also needed for proper interaction with the
EventRewriter, but with the refactoring you're doing, I think we don't have to
modify the X event any more.

This CL does need to be merged though, so I'll look into removing this code in
another CL.

https://codereview.chromium.org/227113009/diff/40001/ash/sticky_keys/sticky_k...
ash/sticky_keys/sticky_keys_controller.cc:516: event->set_flags(event->flags() |
modifier_flag_);
On 2014/04/07 22:32:10, sadrul wrote:
> Since this is not the event you dispatch, do you need to change the flags
here?

There are two sticky key states, normal (press modifier key once) and locked
(press modifier key twice). In the normal case, we redispatch a new event;
however, in the locked case, we modify the event directly and let it continue
with the dispatch. The reason for this is that we need to dispatch the modifier
release event in the proper sequence immediately after the modified event.

[sadrul, 2014-04-08 03:01:33]

LGTM

https://codereview.chromium.org/227113009/diff/40001/ash/sticky_keys/sticky_k...
File ash/sticky_keys/sticky_keys_controller.cc (right):

https://codereview.chromium.org/227113009/diff/40001/ash/sticky_keys/sticky_k...
ash/sticky_keys/sticky_keys_controller.cc:491: XEvent* xev =
event->native_event();
On 2014/04/07 22:53:59, Tim Song wrote:
> On 2014/04/07 22:32:10, sadrul wrote:
> > Since you are creating the mouse-event directly from the ui::MouseEvent, do
> you
> > need to mutate the X event anymore?
> 
> The X event modification was also needed for proper interaction with the
> EventRewriter, but with the refactoring you're doing, I think we don't have to
> modify the X event any more.
> 
> This CL does need to be merged though, so I'll look into removing this code in
> another CL.

Perhaps the X-event mutation isn't needed in the other AppendModifer()s too.
Please look into it when you get a chance. Thanks!

[Tim Song, 2014-04-08 16:43:43]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-04-08 16:44:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/227113009/40001

[commit-bot: I haz the power, 2014-04-08 18:57:34]

Change committed as 262465

[tapted, 2014-04-08 19:23:16]

A revert of this CL has been created in
https://codereview.chromium.org/229053002/ by tapted@chromium.org.

The reason for reverting is: Breaks compile on Linux ChromiumOS (Clang dbg) 

.../sticky_keys_unittest.cc:848:3:error: ignoring return value of function
declared with warn_unused_result attribute [-Werror,-Wunused-result]
  dispatcher->OnEventFromSource(kev.get());
(and 2 more)

link:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....

[Tim Song, 2014-04-08 21:06:23]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-04-08 21:06:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/227113009/60001

[commit-bot: I haz the power, 2014-04-08 22:48:16]

Change committed as 262529