MacViews: Implement basic NSEvent -> ui::Event conversion for Mac

MacViews: Implement basic NSEvent -> ui::Event conversion for Mac

Mac currently links events_stub.cc. This change adds an events_mac.mm
allowing basic ui::Event fields to be populated for keyboard and mouse.

To implement CopyNativeEvent on Mac, some objective C code needs to be
called. Part of this change moves CopyNativeEvent from an anonymous
namespace to platform-specific events_foo.cc/mm, and adds a
corresponding ReleaseCopiedNativeEvent.

BUG=366021
TEST=events_unittests

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

[tapted, 2014-04-24 11:43:30]

Hi sadrul, please take a look.

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

https://codereview.chromium.org/251493002/diff/40001/ui/events/event.cc#newco...
ui/events/event.cc:134: native_event_(base::NativeEvent()),
(C++ value initialization removes the need for the memset)

[sadrul, 2014-04-24 19:47:11]

I am not familiar with cocoa. So you should have someone else review that code.
I left a comment about fling/swipe.

https://codereview.chromium.org/251493002/diff/40001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/251493002/diff/40001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:51: return ET_GESTURE_MULTIFINGER_SWIPE;
MULTIFINGER_SWIPE currently represent multi-finger swipe gestures on
touchscreen. We use SCROLL_FLING_START for two-finger flings on touchpad, or a
single-finger fling on touchscreen. I believe you want SCROLL_FLING_START here?

https://codereview.chromium.org/251493002/diff/40001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:89: NOTIMPLEMENTED();  // Point will be in screen
coordinates.
return after this?

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

https://codereview.chromium.org/251493002/diff/40001/ui/events/event_utils.h#...
ui/events/event_utils.h:93: EVENTS_EXPORT void ReleaseCopiedNativeEvent(
You probably don't need to export these two functions.

https://codereview.chromium.org/251493002/diff/40001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/251493002/diff/40001/ui/events/events.gyp#new...
ui/events/events.gyp:334: '../gfx/gfx.gyp:gfx_test_support',
I think this is already in from tfarina@s change.

https://codereview.chromium.org/251493002/diff/40001/ui/events/events_stub.cc
File ui/events/events_stub.cc (right):

https://codereview.chromium.org/251493002/diff/40001/ui/events/events_stub.cc...
ui/events/events_stub.cc:66: NOTREACHED() << "Don't know how to copy
base::NativeEvent for this platform";
This should be NOTIMPLEMENTED() I believe.

[tapted, 2014-04-25 03:44:54]

+rsesek

Thanks Sadrul! Robert - can I borrow your Cocoa expertise for a review?

https://codereview.chromium.org/251493002/diff/40001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/251493002/diff/40001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:51: return ET_GESTURE_MULTIFINGER_SWIPE;
On 2014/04/24 19:47:12, sadrul wrote:
> MULTIFINGER_SWIPE currently represent multi-finger swipe gestures on
> touchscreen. We use SCROLL_FLING_START for two-finger flings on touchpad, or a
> single-finger fling on touchscreen. I believe you want SCROLL_FLING_START
here?

Done.

https://codereview.chromium.org/251493002/diff/40001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:89: NOTIMPLEMENTED();  // Point will be in screen
coordinates.
On 2014/04/24 19:47:12, sadrul wrote:
> return after this?

Done - now returns (0,0) rather than relying on [nil foo] returning zeroes which
would give a -ve y coordinate. (the right answer might be simple, but I want to
land it with tests - maybe needs a hidpi dance).

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

https://codereview.chromium.org/251493002/diff/40001/ui/events/event_utils.h#...
ui/events/event_utils.h:93: EVENTS_EXPORT void ReleaseCopiedNativeEvent(
On 2014/04/24 19:47:12, sadrul wrote:
> You probably don't need to export these two functions.

Done. I realised that too :) but went for export for consistency. Let me know if
you want these somewhere else instead..

https://codereview.chromium.org/251493002/diff/40001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/251493002/diff/40001/ui/events/events.gyp#new...
ui/events/events.gyp:334: '../gfx/gfx.gyp:gfx_test_support',
On 2014/04/24 19:47:12, sadrul wrote:
> I think this is already in from tfarina@s change.

So it is!

https://codereview.chromium.org/251493002/diff/40001/ui/events/events_stub.cc
File ui/events/events_stub.cc (right):

https://codereview.chromium.org/251493002/diff/40001/ui/events/events_stub.cc...
ui/events/events_stub.cc:66: NOTREACHED() << "Don't know how to copy
base::NativeEvent for this platform";
On 2014/04/24 19:47:12, sadrul wrote:
> This should be NOTIMPLEMENTED() I believe.

Done.

[sadrul, 2014-04-25 11:40:27]

lgtm (sans the cocoa review from rsesek@)

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:90: return gfx::Point(0, 0);
You can just do gfx::Point()

https://codereview.chromium.org/251493002/diff/80001/ui/events/events_stub.cc
File ui/events/events_stub.cc (right):

https://codereview.chromium.org/251493002/diff/80001/ui/events/events_stub.cc...
ui/events/events_stub.cc:26: #if !defined(OS_MACOSX)
I think you can remove this now.

[Robert Sesek, 2014-04-25 15:39:23]

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:67: default:
Shouldn't the default case also be NOTIMPLEMENTED()?

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:78: base::TimeDelta EventTimeFromNative(const
base::NativeEvent& native_event) {
It's not clear to me what this function is trying to accomplish.

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac_unittest.mm (right):

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac_unittest.mm:48: EventsMacTest() {}
Don't need if you're not doing work in it.

[tapted, 2014-04-28 07:39:49]

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:67: default:
On 2014/04/25 15:39:23, rsesek wrote:
> Shouldn't the default case also be NOTIMPLEMENTED()?

events_win and events_x do not do this. However, I'm aware of some things I'll
be adding later (with tests :). E.g. quicklook, which needs API forward declares
planned for a follow-up. And that would miss out otherwise.

So added NOTIMPLEMENTED() here (separate so it can be distinguished), and will
revisit when things are more fleshed out. Done.

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:78: base::TimeDelta EventTimeFromNative(const
base::NativeEvent& native_event) {
On 2014/04/25 15:39:23, rsesek wrote:
> It's not clear to me what this function is trying to accomplish.

It's a somewhat paranoid equivalent of

return base::TimeDelta::FromMicroseconds([native_event timestamp] * 1000000);

Removing the integer component before the *1000000 makes the calculation more
precise. However, it's somewhat academic until the uptime approaches 10^15
microseconds (31 years).

I've added a comment, but happy to go with the non-paranoid version too ;)

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:90: return gfx::Point(0, 0);
On 2014/04/25 11:40:28, sadrul wrote:
> You can just do gfx::Point()

Done.

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac_unittest.mm (right):

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac_unittest.mm:48: EventsMacTest() {}
On 2014/04/25 15:39:23, rsesek wrote:
> Don't need if you're not doing work in it.

This is to get around "events_mac_unittest.mm:123:1: error: constructor for
'ui::EventsMacTest_EventFlagsFromNative_Test' must explicitly initialize the
base class 'ui::(anonymous namespace)::EventsMacTest' which does not have a
default constructor"

(DISALLOW_COPY_AND_ASSIGN prevents the compiler-provided default constructor, so
this adds one back).

https://codereview.chromium.org/251493002/diff/80001/ui/events/events_stub.cc
File ui/events/events_stub.cc (right):

https://codereview.chromium.org/251493002/diff/80001/ui/events/events_stub.cc...
ui/events/events_stub.cc:26: #if !defined(OS_MACOSX)
On 2014/04/25 11:40:28, sadrul wrote:
> I think you can remove this now.

Done.

[Robert Sesek, 2014-04-28 18:51:32]

LGTM

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/251493002/diff/80001/ui/events/cocoa/events_m...
ui/events/cocoa/events_mac.mm:78: base::TimeDelta EventTimeFromNative(const
base::NativeEvent& native_event) {
On 2014/04/28 07:39:49, tapted wrote:
> On 2014/04/25 15:39:23, rsesek wrote:
> > It's not clear to me what this function is trying to accomplish.
> 
> It's a somewhat paranoid equivalent of
> 
> return base::TimeDelta::FromMicroseconds([native_event timestamp] * 1000000);
> 
> Removing the integer component before the *1000000 makes the calculation more
> precise. However, it's somewhat academic until the uptime approaches 10^15
> microseconds (31 years).
> 
> I've added a comment, but happy to go with the non-paranoid version too ;)

That makes sense. I don't think it needs to be that paranoid, though correctness
is generally a good thing.

[tapted, 2014-04-28 23:42:38]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-04-28 23:43:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/251493002/120001

[commit-bot: I haz the power, 2014-04-29 02:32:08]

Change committed as 266743