Aura needs event translation for Mac

Aura needs event translation for Mac

Implments translation code from NSEvent to aura::Event.

BUG=107633
TEST=EventsMacTest.*
R=ben@chromium.org, thakis@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=115084

[Nico, 2011-12-19 20:54:48]

http://codereview.chromium.org/8953037/diff/1/base/event_types.h
File base/event_types.h (right):

http://codereview.chromium.org/8953037/diff/1/base/event_types.h#newcode23
base/event_types.h:23: #import <AppKit/NSEvent.h>
isn't "@class NSEvent;" enough?

http://codereview.chromium.org/8953037/diff/1/base/event_types.h#newcode41
base/event_types.h:41: typedef void* NativeEvent;
Why is the concept of "event" something that belongs in base?

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac.mm
File ui/base/cocoa/events_mac.mm (right):

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac.mm#new...
ui/base/cocoa/events_mac.mm:90: event_flags = event_flags | EF_LEFT_BUTTON_DOWN;
mouse up gets "LEFT_BUTTON_DOWN"? o_O

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac.mm#new...
ui/base/cocoa/events_mac.mm:108: gfx::Point EventLocationFromNative(const
base::NativeEvent& native_event) {
Can you (in a different CL) add a comment to this function in ui/base/events.h
saying in which coordinate system the result of this function is supposed to be
in?

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac.mm#new...
ui/base/cocoa/events_mac.mm:129: bool IsMouseEvent(const base::NativeEvent&
native_event) {
Huh, this function is likely the same on all platforms, right? could it be in a
shared file?

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac.mm#new...
ui/base/cocoa/events_mac.mm:139: int GetMouseWheelOffset(const
base::NativeEvent& native_event) {
blech…can you change this function to return x/y instead, for 2d scrolling?

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac_unitte...
File ui/base/cocoa/events_mac_unittest.mm (right):

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac_unitte...
ui/base/cocoa/events_mac_unittest.mm:24: return [NSEvent
mouseEventWithType:event_type
This looks a lot like the stuff in ui/base/test/cocoa_test_event_utils.h … can
you use that?

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac_unitte...
ui/base/cocoa/events_mac_unittest.mm:346: }
should there be a test that converts from native to non-native key code and back
and checks that that results in the original key code?

[Ben Goodger (Google), 2011-12-19 21:04:55]

http://codereview.chromium.org/8953037/diff/1/base/event_types.h
File base/event_types.h (right):

http://codereview.chromium.org/8953037/diff/1/base/event_types.h#newcode41
base/event_types.h:41: typedef void* NativeEvent;
On 2011/12/19 20:54:49, Nico wrote:
> Why is the concept of "event" something that belongs in base?

It's needed for the message loop dispatcher, which dispatches NativeEvents.

[dhollowa, 2011-12-19 23:19:11]

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac.mm
File ui/base/cocoa/events_mac.mm (right):

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac.mm#new...
ui/base/cocoa/events_mac.mm:90: event_flags = event_flags | EF_LEFT_BUTTON_DOWN;
I agree this is a bad name for the enum, but is the correct mapping.  e.g.
NSLeftMouseUp translates to ET_MOUSE_RELEASED + EF_LEFT_BUTTON_DOWN.  If Ben
agrees I'll rename these in a different CL.

On 2011/12/19 20:54:49, Nico wrote:
> mouse up gets "LEFT_BUTTON_DOWN"? o_O

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac.mm#new...
ui/base/cocoa/events_mac.mm:108: gfx::Point EventLocationFromNative(const
base::NativeEvent& native_event) {
I added it in this CL.

On 2011/12/19 20:54:49, Nico wrote:
> Can you (in a different CL) add a comment to this function in ui/base/events.h
> saying in which coordinate system the result of this function is supposed to
be
> in?

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac.mm#new...
ui/base/cocoa/events_mac.mm:129: bool IsMouseEvent(const base::NativeEvent&
native_event) {
Other platforms are able to avoid two-stage tests.  So per-platform
implementations are more optimal.  I'd prefer to leave this as-is for that
reason.

On 2011/12/19 20:54:49, Nico wrote:
> Huh, this function is likely the same on all platforms, right? could it be in
a
> shared file?

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac.mm#new...
ui/base/cocoa/events_mac.mm:139: int GetMouseWheelOffset(const
base::NativeEvent& native_event) {
I've added it to my list.  This is very much a work-in-progress  so I'll be
following up as the work proceeds.

On 2011/12/19 20:54:49, Nico wrote:
> blech…can you change this function to return x/y instead, for 2d scrolling?

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac_unitte...
File ui/base/cocoa/events_mac_unittest.mm (right):

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac_unitte...
ui/base/cocoa/events_mac_unittest.mm:24: return [NSEvent
mouseEventWithType:event_type
Done.

Yes, I've consolidated these with those and made the naming more consistent.

On 2011/12/19 20:54:49, Nico wrote:
> This looks a lot like the stuff in ui/base/test/cocoa_test_event_utils.h … can
> you use that?

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac_unitte...
ui/base/cocoa/events_mac_unittest.mm:346: }
The back-conversion is not used for production code, and would be non-trivial to
do, so I don't think it is worth it.  The gains over what's here would be
marginal.

On 2011/12/19 20:54:49, Nico wrote:
> should there be a test that converts from native to non-native key code and
back
> and checks that that results in the original key code?

[Nico, 2011-12-19 23:30:28]

LGTM.

One comment nit, and one observation below.

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac.mm
File ui/base/cocoa/events_mac.mm (right):

http://codereview.chromium.org/8953037/diff/1/ui/base/cocoa/events_mac.mm#new...
ui/base/cocoa/events_mac.mm:129: bool IsMouseEvent(const base::NativeEvent&
native_event) {
On 2011/12/19 23:19:11, dhollowa wrote:
> Other platforms are able to avoid two-stage tests.  So per-platform
> implementations are more optimal.  I'd prefer to leave this as-is for that
> reason.

My hunch is that this is not performance-sensitive and that sharing this code
would make things overall simpler, but ok.

http://codereview.chromium.org/8953037/diff/2003/ui/base/events.h
File ui/base/events.h (right):

http://codereview.chromium.org/8953037/diff/2003/ui/base/events.h#newcode84
ui/base/events.h:84: // |Point| has the origin at top-left.
top-left of what?

http://codereview.chromium.org/8953037/diff/2003/ui/base/keycodes/keyboard_co...
File ui/base/keycodes/keyboard_code_conversion_mac.mm (right):

http://codereview.chromium.org/8953037/diff/2003/ui/base/keycodes/keyboard_co...
ui/base/keycodes/keyboard_code_conversion_mac.mm:537: KeyboardCode
KeyboardCodeFromNSEvent(NSEvent* event) {
As far as I understand, this will return "undefined" for all non key codes >
0x127. I guess that's ok for the use case?

[Ben Goodger (Google), 2011-12-19 23:52:03]

LGTM also, from a "integration" perspective.

[dhollowa, 2011-12-20 00:05:33]

http://codereview.chromium.org/8953037/diff/2003/ui/base/events.h
File ui/base/events.h (right):

http://codereview.chromium.org/8953037/diff/2003/ui/base/events.h#newcode84
ui/base/events.h:84: // |Point| has the origin at top-left.
On 2011/12/19 23:30:28, Nico wrote:
> top-left of what?

Done.  The "what" is platform-specific so I've provided pointers to the
pertinent code and mappings.

http://codereview.chromium.org/8953037/diff/2003/ui/base/keycodes/keyboard_co...
File ui/base/keycodes/keyboard_code_conversion_mac.mm (right):

http://codereview.chromium.org/8953037/diff/2003/ui/base/keycodes/keyboard_co...
ui/base/keycodes/keyboard_code_conversion_mac.mm:537: KeyboardCode
KeyboardCodeFromNSEvent(NSEvent* event) {
VKEY_UNKNOWN is returned for key codes >= 0x80 if the event type is not key up
or down, yes.  This is intended.

On 2011/12/19 23:30:28, Nico wrote:
> As far as I understand, this will return "undefined" for all non key codes >
> 0x127. I guess that's ok for the use case?