ozone: Use host coordinates in UIControlsOzone

ozone: Use host coordinates in UIControlsOzone

We're dispatching to WindowTreeHost in the wrong space. Looks like the
transform is generally identity in tests, but let's still make sure to
use the right space.

BUG=none
TEST=interactive_ui_tests in chromeos ozone build

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

[spang, 2014-12-03 02:19:46]

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

[sadrul, 2014-12-03 21:33:56]

https://codereview.chromium.org/774043002/diff/1/ui/aura/test/ui_controls_fac...
File ui/aura/test/ui_controls_factory_ozone.cc (right):

https://codereview.chromium.org/774043002/diff/1/ui/aura/test/ui_controls_fac...
ui/aura/test/ui_controls_factory_ozone.cc:228: ui::MouseEvent mouse_event(type,
host_location, native_screen_location,
Confusing as it is, this is actually not right. Looking at LocatedEvents that
created from an X11 event, LocatedEvent::root_location_ =
LocatedEvent::location_ upon construction. When we say |root_location_| in
LocatedEvent parlance, it's actually the location in the 'root event target'
space (so the root-window in aura, root-view in views), and |location_| is in
the 'event target' space (so the Window or View the event is being dispatched
to).

[spang, 2014-12-03 22:05:01]

On 2014/12/03 21:33:56, sadrul wrote:
>
https://codereview.chromium.org/774043002/diff/1/ui/aura/test/ui_controls_fac...
> File ui/aura/test/ui_controls_factory_ozone.cc (right):
> 
>
https://codereview.chromium.org/774043002/diff/1/ui/aura/test/ui_controls_fac...
> ui/aura/test/ui_controls_factory_ozone.cc:228: ui::MouseEvent
mouse_event(type,
> host_location, native_screen_location,
> Confusing as it is, this is actually not right. Looking at LocatedEvents that
> created from an X11 event, LocatedEvent::root_location_ =
> LocatedEvent::location_ upon construction. When we say |root_location_| in
> LocatedEvent parlance, it's actually the location in the 'root event target'
> space (so the root-window in aura, root-view in views), and |location_| is in
> the 'event target' space (so the Window or View the event is being dispatched
> to).

We are not aiming to exactly replicate the X11 platform behavior when creating
the event. We are merely aiming for correct behavior. Do you think way is
actually better?

This way seems pretty consistent with your description: the "root" window for
the purposes of the desktop environment encompasses the entire native screen,
and the event target is the platform window. So each location is relative to
those things.

When the event leaves the platform, the coordinates are consistent with the
platform's view of the spaces. Once inside WindowTreeHost, root_location gets
reset and the semantics are as you describe.

[spang, 2014-12-03 22:11:19]

On 2014/12/03 22:05:01, spang wrote:
> On 2014/12/03 21:33:56, sadrul wrote:
> >
>
https://codereview.chromium.org/774043002/diff/1/ui/aura/test/ui_controls_fac...
> > File ui/aura/test/ui_controls_factory_ozone.cc (right):
> > 
> >
>
https://codereview.chromium.org/774043002/diff/1/ui/aura/test/ui_controls_fac...
> > ui/aura/test/ui_controls_factory_ozone.cc:228: ui::MouseEvent
> mouse_event(type,
> > host_location, native_screen_location,
> > Confusing as it is, this is actually not right. Looking at LocatedEvents
that
> > created from an X11 event, LocatedEvent::root_location_ =
> > LocatedEvent::location_ upon construction. When we say |root_location_| in
> > LocatedEvent parlance, it's actually the location in the 'root event target'
> > space (so the root-window in aura, root-view in views), and |location_| is
in
> > the 'event target' space (so the Window or View the event is being
dispatched
> > to).
> 
> We are not aiming to exactly replicate the X11 platform behavior when creating
> the event. We are merely aiming for correct behavior. Do you think way is
> actually better?
> 
> This way seems pretty consistent with your description: the "root" window for
> the purposes of the desktop environment encompasses the entire native screen,
> and the event target is the platform window. So each location is relative to
> those things.
> 
> When the event leaves the platform, the coordinates are consistent with the
> platform's view of the spaces. Once inside WindowTreeHost, root_location gets
> reset and the semantics are as you describe.

Note that our actual freon builds are currently running as I described - passing
root_location == native screen location to PlatformWindowDelegate. This patch is
changing UIControlsOzone to match.

[spang, 2014-12-05 20:31:33]

On 2014/12/03 22:11:19, spang wrote:
> On 2014/12/03 22:05:01, spang wrote:
> > On 2014/12/03 21:33:56, sadrul wrote:
> > >
> >
>
https://codereview.chromium.org/774043002/diff/1/ui/aura/test/ui_controls_fac...
> > > File ui/aura/test/ui_controls_factory_ozone.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/774043002/diff/1/ui/aura/test/ui_controls_fac...
> > > ui/aura/test/ui_controls_factory_ozone.cc:228: ui::MouseEvent
> > mouse_event(type,
> > > host_location, native_screen_location,
> > > Confusing as it is, this is actually not right. Looking at LocatedEvents
> that
> > > created from an X11 event, LocatedEvent::root_location_ =
> > > LocatedEvent::location_ upon construction. When we say |root_location_| in
> > > LocatedEvent parlance, it's actually the location in the 'root event
target'
> > > space (so the root-window in aura, root-view in views), and |location_| is
> in
> > > the 'event target' space (so the Window or View the event is being
> dispatched
> > > to).
> > 
> > We are not aiming to exactly replicate the X11 platform behavior when
creating
> > the event. We are merely aiming for correct behavior. Do you think way is
> > actually better?
> > 
> > This way seems pretty consistent with your description: the "root" window
for
> > the purposes of the desktop environment encompasses the entire native
screen,
> > and the event target is the platform window. So each location is relative to
> > those things.
> > 
> > When the event leaves the platform, the coordinates are consistent with the
> > platform's view of the spaces. Once inside WindowTreeHost, root_location
gets
> > reset and the semantics are as you describe.
> 
> Note that our actual freon builds are currently running as I described -
passing
> root_location == native screen location to PlatformWindowDelegate. This patch
is
> changing UIControlsOzone to match.

ping.

I like this way but I need a verdict on whether it is acceptable so we can move
forward.

[sadrul, 2014-12-05 20:36:19]

On 2014/12/03 22:11:19, spang wrote:
> On 2014/12/03 22:05:01, spang wrote:
> > On 2014/12/03 21:33:56, sadrul wrote:
> > >
> >
>
https://codereview.chromium.org/774043002/diff/1/ui/aura/test/ui_controls_fac...
> > > File ui/aura/test/ui_controls_factory_ozone.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/774043002/diff/1/ui/aura/test/ui_controls_fac...
> > > ui/aura/test/ui_controls_factory_ozone.cc:228: ui::MouseEvent
> > mouse_event(type,
> > > host_location, native_screen_location,
> > > Confusing as it is, this is actually not right. Looking at LocatedEvents
> that
> > > created from an X11 event, LocatedEvent::root_location_ =
> > > LocatedEvent::location_ upon construction. When we say |root_location_| in
> > > LocatedEvent parlance, it's actually the location in the 'root event
target'
> > > space (so the root-window in aura, root-view in views), and |location_| is
> in
> > > the 'event target' space (so the Window or View the event is being
> dispatched
> > > to).
> > 
> > We are not aiming to exactly replicate the X11 platform behavior when
creating
> > the event. We are merely aiming for correct behavior. Do you think way is
> > actually better?
> > 
> > This way seems pretty consistent with your description: the "root" window
for
> > the purposes of the desktop environment encompasses the entire native
screen,
> > and the event target is the platform window. So each location is relative to
> > those things.
> > 
> > When the event leaves the platform, the coordinates are consistent with the
> > platform's view of the spaces. Once inside WindowTreeHost, root_location
gets
> > reset and the semantics are as you describe.
> 
> Note that our actual freon builds are currently running as I described -
passing
> root_location == native screen location to PlatformWindowDelegate. This patch
is
> changing UIControlsOzone to match.

We should follow what we are currently doing on chromeos/x11. Otherwise, we risk
introducing regressions in all the places that uses
|LocatedEvent::root_location()|. X11 sets root_location in the WindowTreeHost's
coordinate space, and uses ScreenPositionClient to convert it to screen space.
There's no reason for ozone to be different here. If we want to switch to
storing native-screen location in LocatedEvent instead, then we should do it
everywhere.

[spang, 2014-12-05 21:54:20]

On 2014/12/05 20:36:19, sadrul wrote:
> On 2014/12/03 22:11:19, spang wrote:
> > On 2014/12/03 22:05:01, spang wrote:
> > > On 2014/12/03 21:33:56, sadrul wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/774043002/diff/1/ui/aura/test/ui_controls_fac...
> > > > File ui/aura/test/ui_controls_factory_ozone.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/774043002/diff/1/ui/aura/test/ui_controls_fac...
> > > > ui/aura/test/ui_controls_factory_ozone.cc:228: ui::MouseEvent
> > > mouse_event(type,
> > > > host_location, native_screen_location,
> > > > Confusing as it is, this is actually not right. Looking at LocatedEvents
> > that
> > > > created from an X11 event, LocatedEvent::root_location_ =
> > > > LocatedEvent::location_ upon construction. When we say |root_location_|
in
> > > > LocatedEvent parlance, it's actually the location in the 'root event
> target'
> > > > space (so the root-window in aura, root-view in views), and |location_|
is
> > in
> > > > the 'event target' space (so the Window or View the event is being
> > dispatched
> > > > to).
> > > 
> > > We are not aiming to exactly replicate the X11 platform behavior when
> creating
> > > the event. We are merely aiming for correct behavior. Do you think way is
> > > actually better?
> > > 
> > > This way seems pretty consistent with your description: the "root" window
> for
> > > the purposes of the desktop environment encompasses the entire native
> screen,
> > > and the event target is the platform window. So each location is relative
to
> > > those things.
> > > 
> > > When the event leaves the platform, the coordinates are consistent with
the
> > > platform's view of the spaces. Once inside WindowTreeHost, root_location
> gets
> > > reset and the semantics are as you describe.
> > 
> > Note that our actual freon builds are currently running as I described -
> passing
> > root_location == native screen location to PlatformWindowDelegate. This
patch
> is
> > changing UIControlsOzone to match.
> 
> We should follow what we are currently doing on chromeos/x11. Otherwise, we
risk
> introducing regressions in all the places that uses
> |LocatedEvent::root_location()|. X11 sets root_location in the
WindowTreeHost's
> coordinate space, and uses ScreenPositionClient to convert it to screen space.
> There's no reason for ozone to be different here. If we want to switch to
> storing native-screen location in LocatedEvent instead, then we should do it
> everywhere.

I've made this change.

There were no regressions for code after EventProcessor because root_location is
reset inside OnEventProcessingStarted (via Event::UpdateForRootTransform).

We could have had regressions in EventRewriters - I didn't realize this was
prior to transform. However there aren't any that use root_location(), so
nothing broke.

[sadrul, 2014-12-11 14:58:40]

lgtm

[spang, 2014-12-12 18:48:08]

The CQ bit was checked by spang@chromium.org

[commit-bot: I haz the power, 2014-12-12 18:49:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/774043002/20001

[commit-bot: I haz the power, 2014-12-12 20:16:02]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-12 20:16:59]

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