MacViews: Add WidgetEventGenerator to abstract platform-specific event generation for tests.

MacViews: Enhance aura's EventGenerator to abstract platform-specific event generation for tests.

The focus of this CL is to get widget_unittest.cc compiling on MacViews.

To achieve this, aura::test::EventGenerator is moved to ui/events/test
and constructed with a delegate class that abstracts away the
platform-specific event targeting and coordinate conversions.

ui::test::EventGenerator::CreateDefaultPlatformDelegate(..) allows a default
platform-specific EventGeneratorDelegate to be provided so that
this detail is hidden from tests.

On Mac, this delegate wraps a single NSWindow and targets NSEvents at
that window. On Aura, the delegate is a thin wrapper to find and target
events at an aura::Window, using the aura root window as the initial
target.

For Mac, NativeWidgetPrivate::IsMouseButtonDown and NSView mouse event
handling in BridgedContentView is implemented to put the Mac event
generator in context.

Adds views_unittest WidgetTest.MouseEventTypesViaGenerator to ensure
event generation is consistent across platforms (which currently passes
on Mac). Allows an additional 67 tests to compile on Mac (49 pass).

After this, widget_unittests.cc compiles on Mac. views_unittests has:
Before: 358 tests run 10 tests failed 16 tests crashed.
After: 425 tests run 24 tests failed 20 tests crashed.

BUG=378134
TEST=views_unittests

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

[Andre, 2014-06-17 21:17:57]

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_eve...
File ui/views/test/widget_event_generator.h (right):

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_eve...
ui/views/test/widget_event_generator.h:16: #if defined(OS_MACOSX)
The typedef below uses #elif, why is it different here?

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_eve...
File ui/views/test/widget_event_generator_mac.h (right):

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_eve...
ui/views/test/widget_event_generator_mac.h:25: explicit
WidgetEventGeneratorMac(NSWindow* ns_window);
Should be gfx::NativeWindow to match ns_window_ below?

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_eve...
File ui/views/test/widget_event_generator_mac.mm (right):

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_eve...
ui/views/test/widget_event_generator_mac.mm:158: [responder mouseDown:event];
The target of mouse events should be the view under the mouse, not
firstResponder.
We can use -[NSView hitTest:] to find that view.

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_tes...
File ui/views/test/widget_test_mac.mm (right):

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_tes...
ui/views/test/widget_test_mac.mm:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
2014

https://codereview.chromium.org/322893005/diff/80001/ui/views/widget/widget_u...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/322893005/diff/80001/ui/views/widget/widget_u...
ui/views/widget/widget_unittest.cc:2323: // Test mouse move events.
In Cocoa, mouseEntered: and mouseExited: are sent when entering/exiting
NSTrackingArea not NSView.
mouseEntered: and mouseExited: are sent to the owner of the tracking area.
So the scenario in this test will not happen when moving a real mouse over
views.

https://codereview.chromium.org/322893005/diff/80001/ui/views/widget/widget_u...
ui/views/widget/widget_unittest.cc:2341: EXPECT_EQ(2,
view->GetEventCount(ui::ET_MOUSE_MOVED));
MOUSE_MOVED does not increase to 3 here?
Seems a little weird that entering increments MOUSE_MOVED but not exiting.
Also, check that MOUSE_ENTERED is still 1 here?

[tapted, 2014-06-18 08:28:06]

split off some stuff into http://crrev.com/334573008/ and
http://crrev.com/346463003/.

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_eve...
File ui/views/test/widget_event_generator.h (right):

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_eve...
ui/views/test/widget_event_generator.h:16: #if defined(OS_MACOSX)
On 2014/06/17 21:17:56, Andre wrote:
> The typedef below uses #elif, why is it different here?

Done.

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_eve...
File ui/views/test/widget_event_generator_mac.h (right):

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_eve...
ui/views/test/widget_event_generator_mac.h:25: explicit
WidgetEventGeneratorMac(NSWindow* ns_window);
On 2014/06/17 21:17:56, Andre wrote:
> Should be gfx::NativeWindow to match ns_window_ below?

Done.

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_eve...
File ui/views/test/widget_event_generator_mac.mm (right):

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_eve...
ui/views/test/widget_event_generator_mac.mm:158: [responder mouseDown:event];
On 2014/06/17 21:17:56, Andre wrote:
> The target of mouse events should be the view under the mouse, not
> firstResponder.
> We can use -[NSView hitTest:] to find that view.

Hm good point. Although hitTest is just for mousedown, then the dispatcher
should probably try to keep track of it. views::internal::RootView already does
this. I'll put a comment about it - I don't think the test code needs to
replicate this (at least not yet).

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_tes...
File ui/views/test/widget_test_mac.mm (right):

https://codereview.chromium.org/322893005/diff/80001/ui/views/test/widget_tes...
ui/views/test/widget_test_mac.mm:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
On 2014/06/17 21:17:56, Andre wrote:
> 2014

Done.

https://codereview.chromium.org/322893005/diff/80001/ui/views/widget/widget_u...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/322893005/diff/80001/ui/views/widget/widget_u...
ui/views/widget/widget_unittest.cc:2323: // Test mouse move events.
On 2014/06/17 21:17:56, Andre wrote:
> In Cocoa, mouseEntered: and mouseExited: are sent when entering/exiting
> NSTrackingArea not NSView.
> mouseEntered: and mouseExited: are sent to the owner of the tracking area.
> So the scenario in this test will not happen when moving a real mouse over
> views.

Yeah - EmulateSendEvent in the event generator currently assumes tracking areas
are set up correctly. We'll need separate tests for that in
bridged_somethingorother_unittest to ensure we get mouseMoved events.
views::internal::RootView will then convert the mouseMoved into appropriate
entered/existed events. (so, since the generator shouldn't care about
entered/exited, I've made those NOTREACHED() in EmulateSendEvent)

https://codereview.chromium.org/322893005/diff/80001/ui/views/widget/widget_u...
ui/views/widget/widget_unittest.cc:2341: EXPECT_EQ(2,
view->GetEventCount(ui::ET_MOUSE_MOVED));
On 2014/06/17 21:17:56, Andre wrote:
> MOUSE_MOVED does not increase to 3 here?
> Seems a little weird that entering increments MOUSE_MOVED but not exiting.

I think that's reasonable - if a MOUSEMOVED were sent, it would have coordinates
that were not enclosed within the view, which isn't the case when entering.

> Also, check that MOUSE_ENTERED is still 1 here?

Done.

[tapted, 2014-06-18 08:38:27]

Hi Scott and Robert - please take a look. Hopefully the idea here is not crazy
:). The idea is to allow the same event generation interface to work on aura and
macviews. However the requirements are a bit different when not testing
window-management stuff.

Note I'm not sure about the inheritance direction in WidgetEventGenerator
(ideally WidgetEventGeneratorMac and aura::test::EventGenerator would share some
of their code), but I wanted to get your ideas first rather than
over-engineering it (and maybe this is fine for a test harness).

(tryjobs for the combined CLs currently in crrev/345513002)

[Robert Sesek, 2014-06-18 16:43:02]

https://codereview.chromium.org/322893005/diff/200001/ui/views/test/widget_ev...
File ui/views/test/widget_event_generator_mac.h (right):

https://codereview.chromium.org/322893005/diff/200001/ui/views/test/widget_ev...
ui/views/test/widget_event_generator_mac.h:23: class WidgetEventGeneratorMac {
Could this be made an interface implemented by both Mac and Aura? That may make
it more clear why you have this collection of methods.

https://codereview.chromium.org/322893005/diff/200001/ui/views/test/widget_ev...
File ui/views/test/widget_event_generator_mac.mm (right):

https://codereview.chromium.org/322893005/diff/200001/ui/views/test/widget_ev...
ui/views/test/widget_event_generator_mac.mm:97: return left;
I'd test for left and right and then return other. What if a mouse has four
buttons (left, middle, right, side)?

[sky, 2014-06-18 17:18:01]

sky->ben (to balance my load)

[tapted, 2014-06-19 11:23:21]

https://codereview.chromium.org/322893005/diff/200001/ui/views/test/widget_ev...
File ui/views/test/widget_event_generator_mac.h (right):

https://codereview.chromium.org/322893005/diff/200001/ui/views/test/widget_ev...
ui/views/test/widget_event_generator_mac.h:23: class WidgetEventGeneratorMac {
On 2014/06/18 16:43:02, rsesek wrote:
> Could this be made an interface implemented by both Mac and Aura? That may
make
> it more clear why you have this collection of methods.

Done.

I've just moved out the bits shared so far, rather than try to predict how it
will be used outside the aura context.

https://codereview.chromium.org/322893005/diff/200001/ui/views/test/widget_ev...
File ui/views/test/widget_event_generator_mac.mm (right):

https://codereview.chromium.org/322893005/diff/200001/ui/views/test/widget_ev...
ui/views/test/widget_event_generator_mac.mm:97: return left;
On 2014/06/18 16:43:02, rsesek wrote:
> I'd test for left and right and then return other. What if a mouse has four
> buttons (left, middle, right, side)?

Done.

[Robert Sesek, 2014-06-19 13:55:49]

cocoa bits LGTM

[Andre, 2014-06-19 19:10:52]

lgtm

[tapted, 2014-06-19 23:46:59]

ben (ping): PTAL for OWNERS when you get a chance

[tapted, 2014-06-23 22:08:08]

ben: ping? (OWNERS)

[tapted, 2014-07-09 22:45:27]

ben: hopeful ping?

[Ben Goodger (Google), 2014-07-11 18:26:50]

Hey sadrul check out my comment below.

The app list guys want to use the event generator from a views-only no-aura
environment. I think rather than splitting the event generator as they're doing,
it might be better to try to move event generator to ui/events. WDYT?

https://codereview.chromium.org/322893005/diff/280001/ui/aura/test/event_gene...
File ui/aura/test/event_generator.h (right):

https://codereview.chromium.org/322893005/diff/280001/ui/aura/test/event_gene...
ui/aura/test/event_generator.h:307: WindowTreeHost* current_host_;
so if I understand this correctly the issue you're having here is that this
class is aura-specific and you want to use it from views. so you extract a base
class & a new specialization in views.

I think a better approach might be to move all of this code into
src/ui/events/test and remove all the aura-specific bits that way. ui/events
already has a number of abstractions implemented in both aura and views for this
specific reason (e.g. PlatformEventSource, et al.)

It seems like every method on this class that takes aura::Window* could for
example take ui::EventTarget*. And so on.

+sadrul to see if he has any further thoughts.

https://codereview.chromium.org/322893005/diff/280001/ui/base/test/event_gene...
File ui/base/test/event_generator_base.h (right):

https://codereview.chromium.org/322893005/diff/280001/ui/base/test/event_gene...
ui/base/test/event_generator_base.h:5: #ifndef
UI_BASE_TEST_EVENT_GENERATOR_BASE_H_
I think this should go into ui/events/test rather than ui/base/test.

[sadrul, 2014-07-12 07:10:54]

https://codereview.chromium.org/322893005/diff/280001/ui/aura/test/event_gene...
File ui/aura/test/event_generator.h (right):

https://codereview.chromium.org/322893005/diff/280001/ui/aura/test/event_gene...
ui/aura/test/event_generator.h:307: WindowTreeHost* current_host_;
On 2014/07/11 18:26:49, Ben Goodger (Google) wrote:
> so if I understand this correctly the issue you're having here is that this
> class is aura-specific and you want to use it from views. so you extract a
base
> class & a new specialization in views.
> 
> I think a better approach might be to move all of this code into
> src/ui/events/test and remove all the aura-specific bits that way. ui/events
> already has a number of abstractions implemented in both aura and views for
this
> specific reason (e.g. PlatformEventSource, et al.)
> 
> It seems like every method on this class that takes aura::Window* could for
> example take ui::EventTarget*. And so on.

I think this is a good idea! Having a generic EventGenerator (that works with
EventTarget, EventSource etc.) in //ui/events/test/ would be nice. We will
likely need some overrides for aura (and maybe eventually for non-aura views),
but that should be possible.

> 
> +sadrul to see if he has any further thoughts.

[tapted, 2014-07-14 11:37:47]

wow - PTAL. I think this turned out pretty neat :)

I fleshed out the EventGenerator delegate interface to abstract the Aura
specifics. For Mac at this stage, the delegate sucks in all events and redirects
them at the NSWindow using native events to get appropriate coverage (currently:
just mouse events).

For now, I didn't have to override anything on the EventGenerator itself - the
delegate is able to draw a nice line that separates the platform-specific
coordinate conversion and screen->EventTarget mapping from the EventGenerator.
EventGenerator{Mac,Aura} are essentially just helper classes to construct the
delegate. Some of the boilerplate on EventGeneratorMac from earlier patchsets
could also be scrapped, without affecting tests pass/fail stats on Mac \o/.

Note that we can now also remove a lot of `ui::` prefixes from the
EventGenerator implementation, but this generated a lot of whitespace noise that
confused the diff, so I backed it out. I'll do this in a follow-up CL (follow-up
patchset if you prefer).

I've also held off on renaming too much. E.g. it might be nice to rename
aura::test::EventGenerator to EventGeneratorAura, so it's harder to confuse with
ui::test::EventGenerator. But it works OK as it is too.

https://codereview.chromium.org/322893005/diff/280001/ui/aura/test/event_gene...
File ui/aura/test/event_generator.h (right):

https://codereview.chromium.org/322893005/diff/280001/ui/aura/test/event_gene...
ui/aura/test/event_generator.h:307: WindowTreeHost* current_host_;
On 2014/07/12 07:10:53, sadrul wrote:
> On 2014/07/11 18:26:49, Ben Goodger (Google) wrote:
> > so if I understand this correctly the issue you're having here is that this
> > class is aura-specific and you want to use it from views. so you extract a
> base
> > class & a new specialization in views.
> > 
> > I think a better approach might be to move all of this code into
> > src/ui/events/test and remove all the aura-specific bits that way. ui/events
> > already has a number of abstractions implemented in both aura and views for
> this
> > specific reason (e.g. PlatformEventSource, et al.)
> > 
> > It seems like every method on this class that takes aura::Window* could for
> > example take ui::EventTarget*. And so on.
> 
> I think this is a good idea! Having a generic EventGenerator (that works with
> EventTarget, EventSource etc.) in //ui/events/test/ would be nice. We will
> likely need some overrides for aura (and maybe eventually for non-aura views),
> but that should be possible.
> 
> > 
> > +sadrul to see if he has any further thoughts.
> 

Done.

https://codereview.chromium.org/322893005/diff/280001/ui/base/test/event_gene...
File ui/base/test/event_generator_base.h (right):

https://codereview.chromium.org/322893005/diff/280001/ui/base/test/event_gene...
ui/base/test/event_generator_base.h:5: #ifndef
UI_BASE_TEST_EVENT_GENERATOR_BASE_H_
On 2014/07/11 18:26:50, Ben Goodger (Google) wrote:
> I think this should go into ui/events/test rather than ui/base/test.

Done.

[tapted, 2014-07-14 11:42:12]

https://codereview.chromium.org/322893005/diff/400001/ui/aura/test/event_gene...
File ui/aura/test/event_generator.cc (right):

https://codereview.chromium.org/322893005/diff/400001/ui/aura/test/event_gene...
ui/aura/test/event_generator.cc:37: class TestKeyEvent : public ui::KeyEvent {
oops - this and TestTouchEvent are no longer needed here. I'll remove them.

[tapted, 2014-07-15 12:43:27]

(also rebased for a WidgetTest added in r283070 that used EventGenerator and
would want a header this CL takes away. fixed in patchset 9 - all looks good).

https://codereview.chromium.org/322893005/diff/400001/ui/aura/test/event_gene...
File ui/aura/test/event_generator.cc (right):

https://codereview.chromium.org/322893005/diff/400001/ui/aura/test/event_gene...
ui/aura/test/event_generator.cc:37: class TestKeyEvent : public ui::KeyEvent {
On 2014/07/14 11:42:12, tapted wrote:
> oops - this and TestTouchEvent are no longer needed here. I'll remove them.

Done.

[Ben Goodger (Google), 2014-07-15 16:22:20]

https://codereview.chromium.org/322893005/diff/480001/ui/views/test/widget_ev...
File ui/views/test/widget_event_generator.h (right):

https://codereview.chromium.org/322893005/diff/480001/ui/views/test/widget_ev...
ui/views/test/widget_event_generator.h:33: class WidgetEventGenerator : public
PlatformEventGenerator {
Now that you've managed to hide the platform differences almost entirely behind
EventGeneratorDelegate, can you try to avoid subclassing EventGenerator?

Inheritance in non-interface framework types gives me the heebee-jeebees.

[tapted, 2014-07-16 07:56:07]

https://codereview.chromium.org/322893005/diff/480001/ui/views/test/widget_ev...
File ui/views/test/widget_event_generator.h (right):

https://codereview.chromium.org/322893005/diff/480001/ui/views/test/widget_ev...
ui/views/test/widget_event_generator.h:33: class WidgetEventGenerator : public
PlatformEventGenerator {
On 2014/07/15 16:22:20, Ben Goodger (Google) wrote:
> Now that you've managed to hide the platform differences almost entirely
behind
> EventGeneratorDelegate, can you try to avoid subclassing EventGenerator?
> 
> Inheritance in non-interface framework types gives me the heebee-jeebees.

Done. Took some juggling, but getting rid of these helper classes got rid of a
lot of boring code (3 files - widget_event_generator{.h,_aura.h,_aura.cc} - have
dropped off the review).

I think ui/views/test/widget_event_generator_mac.mm now needs a different name
and could potentially go somewhere else (ui/events/test even). I've kept it
where it is for now so the patchset diff doesn't break. (The Widget dependency
is gone, but the conservative part of me wants to just rename it to
event_generator_mac.mm in case it needs views stuff again..).

[Ben Goodger (Google), 2014-07-16 15:05:14]

This is an awesome change and a huge improvement to EventGenerator. Thanks for
doing this!

LGTM. See file naming suggestions below:

https://codereview.chromium.org/322893005/diff/620001/ui/aura/test/event_gene...
File ui/aura/test/event_generator.h (right):

https://codereview.chromium.org/322893005/diff/620001/ui/aura/test/event_gene...
ui/aura/test/event_generator.h:5: #ifndef UI_AURA_TEST_EVENT_GENERATOR_H_
TODO: rename this file event_generator_delegate_aura.*

https://codereview.chromium.org/322893005/diff/620001/ui/views/test/widget_ev...
File ui/views/test/widget_event_generator_mac.mm (right):

https://codereview.chromium.org/322893005/diff/620001/ui/views/test/widget_ev...
ui/views/test/widget_event_generator_mac.mm:3: // found in the LICENSE file.
i would just call this file event_generator_delegate_mac.mm or
widget_event_generator_delegate_mac.mm

[tapted, 2014-07-17 03:42:34]

Thanks all!

https://codereview.chromium.org/322893005/diff/620001/ui/aura/test/event_gene...
File ui/aura/test/event_generator.h (right):

https://codereview.chromium.org/322893005/diff/620001/ui/aura/test/event_gene...
ui/aura/test/event_generator.h:5: #ifndef UI_AURA_TEST_EVENT_GENERATOR_H_
On 2014/07/16 15:05:14, Ben Goodger (Google) wrote:
> TODO: rename this file event_generator_delegate_aura.*

Done.

https://codereview.chromium.org/322893005/diff/620001/ui/views/test/widget_ev...
File ui/views/test/widget_event_generator_mac.mm (right):

https://codereview.chromium.org/322893005/diff/620001/ui/views/test/widget_ev...
ui/views/test/widget_event_generator_mac.mm:3: // found in the LICENSE file.
On 2014/07/16 15:05:14, Ben Goodger (Google) wrote:
> i would just call this file event_generator_delegate_mac.mm or
> widget_event_generator_delegate_mac.mm

Done (former).

[tapted, 2014-07-17 03:42:57]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-07-17 03:46:28]

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

[commit-bot: I haz the power, 2014-07-17 07:36:44]

Change committed as 283694

[sadrul, 2014-07-17 16:18:51]

Very nice! LGTM!

[tfarina, 2014-07-31 00:20:19]

https://codereview.chromium.org/322893005/diff/640001/ui/events/test/event_ge...
File ui/events/test/event_generator.cc (right):

https://codereview.chromium.org/322893005/diff/640001/ui/events/test/event_ge...
ui/events/test/event_generator.cc:62: :
delegate_(CreateDefaultPlatformDelegate(this, root_window, NULL)),
So now, with this, events_test_support depends on aura_test_support since the
implementation is in ui/aura/test/event_generator_delegate_aura.cc and
views_test_support as the implementation is in
ui/views/test/event_generator_delegate_mac.mm

I think that is why my GN patch is breaking and was reverted in
https://codereview.chromium.org/427403003/.

I don't know this went unnoticed and that you don't have aura_test_suport and
views_test_support in events.gyp.

[tfarina, 2014-07-31 00:49:24]

https://codereview.chromium.org/322893005/diff/640001/ui/events/test/event_ge...
File ui/events/test/event_generator.cc (right):

https://codereview.chromium.org/322893005/diff/640001/ui/events/test/event_ge...
ui/events/test/event_generator.cc:62: :
delegate_(CreateDefaultPlatformDelegate(this, root_window, NULL)),
On 2014/07/31 00:20:19, tfarina wrote:
> So now, with this, events_test_support depends on aura_test_support since the
> implementation is in ui/aura/test/event_generator_delegate_aura.cc and
> views_test_support as the implementation is in
> ui/views/test/event_generator_delegate_mac.mm
> 
> I think that is why my GN patch is breaking and was reverted in
> https://codereview.chromium.org/427403003/.
> 
> I don't know this went unnoticed and that you don't have aura_test_suport and
> views_test_support in events.gyp.
> 
> 
And if we try to depend on either it will create a circular dependency.

See my comments in crbug.com/399106

How come this is passing in the GYP build is escaping me. (I might be wrong?)

[brettw, 2014-07-31 17:45:04]

I only looked briefly, but it looks like Thiago is correct and that the layering
is wrong here. We we prioritize cleaning this up?

[tapted, 2014-07-31 23:17:04]

On 2014/07/31 17:45:04, brettw wrote:
> I only looked briefly, but it looks like Thiago is correct and that the
layering
> is wrong here. We we prioritize cleaning this up?

I followed up in http://crbug.com/399106 with a short-term fix. I agree this is
not ideal. The test_support target gets away with it because it is statically
linked, but the GN file didn't have it that way. I think I have a fix for the
layering that's neater than the helper classes I had here in patchset 9. I'll
try it out.