Reland of: Remove deprecated cursor event filter for Ozone

Reland of: Remove deprecated cursor event filter for Ozone

Initial code review: https://codereview.chromium.org/580553002/

BUG=none

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

[dnicoara, 2014-09-17 20:54:02]

dnicoara@chromium.org changed reviewers:
+ oshima@chromium.org

[dnicoara, 2014-09-17 20:54:03]

PTAL

Patch 1 is the previous version and patch 2 is the update.

Must have used the wrong build when testing before.

https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
File ash/display/mouse_cursor_event_filter.cc (right):

https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
ash/display/mouse_cursor_event_filter.cc:238:
point_in_native.Offset(target->GetHost()->GetBounds().x(),
Is there a nicer way to do this?

[oshima, 2014-09-17 21:36:49]

https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
File ash/display/mouse_cursor_event_filter.cc (right):

https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
ash/display/mouse_cursor_event_filter.cc:238:
point_in_native.Offset(target->GetHost()->GetBounds().x(),
On 2014/09/17 20:54:03, dnicoara wrote:
> Is there a nicer way to do this?

so the host bounds is in fb's coordinates, and the native point is in host
coordinates?

I don't have strong opinion here, but the best way is to handle mouse warping in
ozone, rather than in chrome :)

The nicer way would be to fix EventSystemLocationFromNative to return the
coordinates in fb. Looks like ozone is using ui::Events, so it should have the
target, and you can ask target (something like
Target::GetSystemLocationForEvent(LocatedEvent) ) to return the correct
location.

Another option is to use
WTH::ConvertPointFromHost() -> WTH::ConvertPointToNativeScreen, although I think
it's overkill.

Can you also move the code like this?

aura::Window* target =...
gfx::Point piont_in_native = ...
#if defined(USE_OZONE)
...
#endif
gfx::Point point_in_screen = ..


I think this is easier to follow.

[spang, 2014-09-17 21:41:03]

On 2014/09/17 21:36:49, oshima wrote:
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> File ash/display/mouse_cursor_event_filter.cc (right):
> 
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> ash/display/mouse_cursor_event_filter.cc:238:
> point_in_native.Offset(target->GetHost()->GetBounds().x(),
> On 2014/09/17 20:54:03, dnicoara wrote:
> > Is there a nicer way to do this?
> 
> so the host bounds is in fb's coordinates, and the native point is in host
> coordinates?
> 
> I don't have strong opinion here, but the best way is to handle mouse warping
in
> ozone, rather than in chrome :)
> 
> The nicer way would be to fix EventSystemLocationFromNative to return the
> coordinates in fb. Looks like ozone is using ui::Events, so it should have the
> target, and you can ask target (something like
> Target::GetSystemLocationForEvent(LocatedEvent) ) to return the correct
> location.
> 
> Another option is to use
> WTH::ConvertPointFromHost() -> WTH::ConvertPointToNativeScreen, although I
think
> it's overkill.
> 
> Can you also move the code like this?
> 
> aura::Window* target =...
> gfx::Point piont_in_native = ...
> #if defined(USE_OZONE)
> ...
> #endif
> gfx::Point point_in_screen = ..
> 
> 
> I think this is easier to follow.

[spang, 2014-09-17 21:53:12]

On 2014/09/17 21:41:03, spang wrote:
> On 2014/09/17 21:36:49, oshima wrote:
> >
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> > File ash/display/mouse_cursor_event_filter.cc (right):
> > 
> >
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> > ash/display/mouse_cursor_event_filter.cc:238:
> > point_in_native.Offset(target->GetHost()->GetBounds().x(),
> > On 2014/09/17 20:54:03, dnicoara wrote:
> > > Is there a nicer way to do this?
> > 
> > so the host bounds is in fb's coordinates, and the native point is in host
> > coordinates?
> > 
> > I don't have strong opinion here, but the best way is to handle mouse
warping
> in
> > ozone, rather than in chrome :)
> > 
> > The nicer way would be to fix EventSystemLocationFromNative to return the
> > coordinates in fb. Looks like ozone is using ui::Events, so it should have
the
> > target, and you can ask target (something like
> > Target::GetSystemLocationForEvent(LocatedEvent) ) to return the correct
> > location.

(oops - sent empty msg)

I don't think we can implement EventSystemLocationFromNative easily. We should
make it NOTREACHED() and fix all the users.

We don't even have a field in th to store a "screen" location - this needs to be
calculated using ScreenPositionClient or similar if it's necessary.

> > 
> > Another option is to use
> > WTH::ConvertPointFromHost() -> WTH::ConvertPointToNativeScreen, although I
> think
> > it's overkill.
> > 
> > Can you also move the code like this?
> > 
> > aura::Window* target =...
> > gfx::Point piont_in_native = ...
> > #if defined(USE_OZONE)
> > ...
> > #endif
> > gfx::Point point_in_screen = ..
> > 
> > 
> > I think this is easier to follow.

[oshima, 2014-09-17 22:10:18]

On 2014/09/17 21:53:12, spang wrote:
> On 2014/09/17 21:41:03, spang wrote:
> > On 2014/09/17 21:36:49, oshima wrote:
> > >
> >
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> > > File ash/display/mouse_cursor_event_filter.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> > > ash/display/mouse_cursor_event_filter.cc:238:
> > > point_in_native.Offset(target->GetHost()->GetBounds().x(),
> > > On 2014/09/17 20:54:03, dnicoara wrote:
> > > > Is there a nicer way to do this?
> > > 
> > > so the host bounds is in fb's coordinates, and the native point is in host
> > > coordinates?
> > > 
> > > I don't have strong opinion here, but the best way is to handle mouse
> warping
> > in
> > > ozone, rather than in chrome :)
> > > 
> > > The nicer way would be to fix EventSystemLocationFromNative to return the
> > > coordinates in fb. Looks like ozone is using ui::Events, so it should have
> the
> > > target, and you can ask target (something like
> > > Target::GetSystemLocationForEvent(LocatedEvent) ) to return the correct
> > > location.
> 
> (oops - sent empty msg)
> 
> I don't think we can implement EventSystemLocationFromNative easily. We should
> make it NOTREACHED() and fix all the users.
> 
> We don't even have a field in th to store a "screen" location - this needs to
be
> calculated using ScreenPositionClient or similar if it's necessary.

You can delegate it via EventTarget, which will be implemented in aura Window.

> 
> > > 
> > > Another option is to use
> > > WTH::ConvertPointFromHost() -> WTH::ConvertPointToNativeScreen, although I
> > think
> > > it's overkill.
> > > 
> > > Can you also move the code like this?
> > > 
> > > aura::Window* target =...
> > > gfx::Point piont_in_native = ...
> > > #if defined(USE_OZONE)
> > > ...
> > > #endif
> > > gfx::Point point_in_screen = ..
> > > 
> > > 
> > > I think this is easier to follow.

[dnicoara, 2014-09-18 14:40:43]

On 2014/09/17 21:36:49, oshima wrote:
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> File ash/display/mouse_cursor_event_filter.cc (right):
> 
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> ash/display/mouse_cursor_event_filter.cc:238:
> point_in_native.Offset(target->GetHost()->GetBounds().x(),
> On 2014/09/17 20:54:03, dnicoara wrote:
> > Is there a nicer way to do this?
> 
> so the host bounds is in fb's coordinates, and the native point is in host
> coordinates?
> 
> I don't have strong opinion here, but the best way is to handle mouse warping
in
> ozone, rather than in chrome :)

I agree, but the Ozone layer doesn't have access to the logical layout of the
displays.

> 
> The nicer way would be to fix EventSystemLocationFromNative to return the
> coordinates in fb. Looks like ozone is using ui::Events, so it should have the
> target, and you can ask target (something like
> Target::GetSystemLocationForEvent(LocatedEvent) ) to return the correct
> location.
> 

As spang@ mentioned it is hard to fix EventSystemLocationFromNative since we'd
have to attach a window to a native event. Then aura::Window would have to
perform a conversion that seems to be more platform specific. I don't have a
clear image how this would look like, but it seems overly complicated. Your next
suggestion seems cleaner to me.

> Another option is to use
> WTH::ConvertPointFromHost() -> WTH::ConvertPointToNativeScreen, although I
think
> it's overkill.
> 

I was thinking about this but I agree with you, it is a bit overkill.

Alternatively, wondering if I could use the event directly (rather than the
native event) and convert it to the native screen coordinate system using
WTH::ConvertPointToNativeScreen()? This should work for the X11 case as well,
right? (Also bonus points for not using EventSystemLocationFromNative() at all)

Note can't use ScreenPositionController since the displays are scaled. I could
augment ScreenPositionController to have a ConvertPointToNativeScreen if this
makes sense.

> Can you also move the code like this?
> 
> aura::Window* target =...
> gfx::Point piont_in_native = ...
> #if defined(USE_OZONE)
> ...
> #endif
> gfx::Point point_in_screen = ..
> 
> 
> I think this is easier to follow.

Done

[spang, 2014-09-18 15:43:34]

On 2014/09/18 14:40:43, dnicoara wrote:
> On 2014/09/17 21:36:49, oshima wrote:
> >
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> > File ash/display/mouse_cursor_event_filter.cc (right):
> > 
> >
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> > ash/display/mouse_cursor_event_filter.cc:238:
> > point_in_native.Offset(target->GetHost()->GetBounds().x(),
> > On 2014/09/17 20:54:03, dnicoara wrote:
> > > Is there a nicer way to do this?
> > 
> > so the host bounds is in fb's coordinates, and the native point is in host
> > coordinates?
> > 
> > I don't have strong opinion here, but the best way is to handle mouse
warping
> in
> > ozone, rather than in chrome :)
> 
> I agree, but the Ozone layer doesn't have access to the logical layout of the
> displays.

We should open a bug for this. It's wrong for ash to be laying things out in an
unnatural way under ozone. The display code should be fixed to use the
user-visible layout all the way down into our code.

> 
> > 
> > The nicer way would be to fix EventSystemLocationFromNative to return the
> > coordinates in fb. Looks like ozone is using ui::Events, so it should have
the
> > target, and you can ask target (something like
> > Target::GetSystemLocationForEvent(LocatedEvent) ) to return the correct
> > location.
> > 
> 
> As spang@ mentioned it is hard to fix EventSystemLocationFromNative since we'd
> have to attach a window to a native event. Then aura::Window would have to
> perform a conversion that seems to be more platform specific. I don't have a
> clear image how this would look like, but it seems overly complicated. Your
next
> suggestion seems cleaner to me.
> 
> > Another option is to use
> > WTH::ConvertPointFromHost() -> WTH::ConvertPointToNativeScreen, although I
> think
> > it's overkill.
> > 
> 
> I was thinking about this but I agree with you, it is a bit overkill.
> 
> Alternatively, wondering if I could use the event directly (rather than the
> native event) and convert it to the native screen coordinate system using
> WTH::ConvertPointToNativeScreen()? This should work for the X11 case as well,
> right? (Also bonus points for not using EventSystemLocationFromNative() at
all)
> 
> Note can't use ScreenPositionController since the displays are scaled. I could
> augment ScreenPositionController to have a ConvertPointToNativeScreen if this
> makes sense.
> 
> > Can you also move the code like this?
> > 
> > aura::Window* target =...
> > gfx::Point piont_in_native = ...
> > #if defined(USE_OZONE)
> > ...
> > #endif
> > gfx::Point point_in_screen = ..
> > 
> > 
> > I think this is easier to follow.
> 
> Done

[oshima, 2014-09-18 18:13:08]

On 2014/09/18 15:43:34, spang wrote:
> On 2014/09/18 14:40:43, dnicoara wrote:
> > On 2014/09/17 21:36:49, oshima wrote:
> > >
> >
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> > > File ash/display/mouse_cursor_event_filter.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> > > ash/display/mouse_cursor_event_filter.cc:238:
> > > point_in_native.Offset(target->GetHost()->GetBounds().x(),
> > > On 2014/09/17 20:54:03, dnicoara wrote:
> > > > Is there a nicer way to do this?
> > > 
> > > so the host bounds is in fb's coordinates, and the native point is in host
> > > coordinates?
> > > 
> > > I don't have strong opinion here, but the best way is to handle mouse
> warping
> > in
> > > ozone, rather than in chrome :)
> > 
> > I agree, but the Ozone layer doesn't have access to the logical layout of
the
> > displays.
> 
> We should open a bug for this. It's wrong for ash to be laying things out in
an
> unnatural way under ozone. The display code should be fixed to use the
> user-visible layout all the way down into our code.

Yes. We already push the configuration from ash to ozone via
DisplayConfigurator,
so it should be possible to send the layout information as well. Tricky part is
that
it needs to handle scaling and rotation, but it shouldn't be that hard.

> 
> > 
> > > 
> > > The nicer way would be to fix EventSystemLocationFromNative to return the
> > > coordinates in fb. Looks like ozone is using ui::Events, so it should have
> the
> > > target, and you can ask target (something like
> > > Target::GetSystemLocationForEvent(LocatedEvent) ) to return the correct
> > > location.
> > > 
> > 
> > As spang@ mentioned it is hard to fix EventSystemLocationFromNative since
we'd
> > have to attach a window to a native event.

According to the code
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/ozone/ev...
ozone's native event is ui::LocatedEvent (which I assume the same instance), so
it should have target, shouldn't it?

In any case, I still think the best way is to move the logic to ozone, and
I'm fine with this approach as a short term solution.

> Then aura::Window would have to
> > perform a conversion that seems to be more platform specific. I don't have a
> > clear image how this would look like, but it seems overly complicated. Your
> next
> > suggestion seems cleaner to me.
> > 
> > > Another option is to use
> > > WTH::ConvertPointFromHost() -> WTH::ConvertPointToNativeScreen, although I
> > think
> > > it's overkill.
> > > 
> > 
> > I was thinking about this but I agree with you, it is a bit overkill.
> > 
> > Alternatively, wondering if I could use the event directly (rather than the
> > native event) and convert it to the native screen coordinate system using
> > WTH::ConvertPointToNativeScreen()? This should work for the X11 case as
well,
> > right? (Also bonus points for not using EventSystemLocationFromNative() at
> all)

It used to be that way, which was causing problems due to conversion errors when
the desktop is scaled & rotated.
I changed to use native coordinates to solve those issues.

> > 
> > Note can't use ScreenPositionController since the displays are scaled. I
could
> > augment ScreenPositionController to have a ConvertPointToNativeScreen if
this
> > makes sense.
> > 
> > > Can you also move the code like this?
> > > 
> > > aura::Window* target =...
> > > gfx::Point piont_in_native = ...
> > > #if defined(USE_OZONE)
> > > ...
> > > #endif
> > > gfx::Point point_in_screen = ..
> > > 
> > > 
> > > I think this is easier to follow.
> > 
> > Done

[oshima, 2014-09-18 18:13:27]

lgtm

https://codereview.chromium.org/575143002/diff/40001/ash/display/mouse_cursor...
File ash/display/mouse_cursor_event_filter.cc (right):

https://codereview.chromium.org/575143002/diff/40001/ash/display/mouse_cursor...
ash/display/mouse_cursor_event_filter.cc:235: // to translate them to get the
global position.
Please file a bug and add it here.

[dnicoara, 2014-09-18 19:22:47]

https://codereview.chromium.org/575143002/diff/40001/ash/display/mouse_cursor...
File ash/display/mouse_cursor_event_filter.cc (right):

https://codereview.chromium.org/575143002/diff/40001/ash/display/mouse_cursor...
ash/display/mouse_cursor_event_filter.cc:235: // to translate them to get the
global position.
On 2014/09/18 18:13:27, oshima wrote:
> Please file a bug and add it here.

Done.

[dnicoara, 2014-09-18 19:29:06]

On 2014/09/18 18:13:08, oshima wrote:
> On 2014/09/18 15:43:34, spang wrote:
> > On 2014/09/18 14:40:43, dnicoara wrote:
> > > On 2014/09/17 21:36:49, oshima wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> > > > File ash/display/mouse_cursor_event_filter.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/575143002/diff/20001/ash/display/mouse_cursor...
> > > > ash/display/mouse_cursor_event_filter.cc:238:
> > > > point_in_native.Offset(target->GetHost()->GetBounds().x(),
> > > > On 2014/09/17 20:54:03, dnicoara wrote:
> > > > > Is there a nicer way to do this?
> > > > 
> > > > so the host bounds is in fb's coordinates, and the native point is in
host
> > > > coordinates?
> > > > 
> > > > I don't have strong opinion here, but the best way is to handle mouse
> > warping
> > > in
> > > > ozone, rather than in chrome :)
> > > 
> > > I agree, but the Ozone layer doesn't have access to the logical layout of
> the
> > > displays.
> > 
> > We should open a bug for this. It's wrong for ash to be laying things out in
> an
> > unnatural way under ozone. The display code should be fixed to use the
> > user-visible layout all the way down into our code.
> 
> Yes. We already push the configuration from ash to ozone via
> DisplayConfigurator,
> so it should be possible to send the layout information as well. Tricky part
is
> that
> it needs to handle scaling and rotation, but it shouldn't be that hard.
> 

I think we already wanted to support some rotation operations directly in
hardware so this would be a good opportunity. This could be a good opportunity
to also merge/simplify some of the logic in DisplayManager with that in
DisplayConfigurator. But we can continue the discussion in crbug.com/415680

> > 
> > > 
> > > > 
> > > > The nicer way would be to fix EventSystemLocationFromNative to return
the
> > > > coordinates in fb. Looks like ozone is using ui::Events, so it should
have
> > the
> > > > target, and you can ask target (something like
> > > > Target::GetSystemLocationForEvent(LocatedEvent) ) to return the correct
> > > > location.
> > > > 
> > > 
> > > As spang@ mentioned it is hard to fix EventSystemLocationFromNative since
> we'd
> > > have to attach a window to a native event.
> 
> According to the code
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/ozone/ev...
> ozone's native event is ui::LocatedEvent (which I assume the same instance),
so
> it should have target, shouldn't it?

Unfortunately, for the time being (in order to more easily add features) the
native event is a ui event, but it is a distinct instance (yes, we have a
ui::Event inside a ui::Event) so the native event doesn't have the target.

> 
> In any case, I still think the best way is to move the logic to ozone, and
> I'm fine with this approach as a short term solution.

Yep, I liked this solution better since it drives down the diff by a lot in the
mean time.

> 
> > Then aura::Window would have to
> > > perform a conversion that seems to be more platform specific. I don't have
a
> > > clear image how this would look like, but it seems overly complicated.
Your
> > next
> > > suggestion seems cleaner to me.
> > > 
> > > > Another option is to use
> > > > WTH::ConvertPointFromHost() -> WTH::ConvertPointToNativeScreen, although
I
> > > think
> > > > it's overkill.
> > > > 
> > > 
> > > I was thinking about this but I agree with you, it is a bit overkill.
> > > 
> > > Alternatively, wondering if I could use the event directly (rather than
the
> > > native event) and convert it to the native screen coordinate system using
> > > WTH::ConvertPointToNativeScreen()? This should work for the X11 case as
> well,
> > > right? (Also bonus points for not using EventSystemLocationFromNative() at
> > all)
> 
> It used to be that way, which was causing problems due to conversion errors
when
> the desktop is scaled & rotated.
> I changed to use native coordinates to solve those issues.
> 
Ah, thank you for the background info.

> > > 
> > > Note can't use ScreenPositionController since the displays are scaled. I
> could
> > > augment ScreenPositionController to have a ConvertPointToNativeScreen if
> this
> > > makes sense.
> > > 
> > > > Can you also move the code like this?
> > > > 
> > > > aura::Window* target =...
> > > > gfx::Point piont_in_native = ...
> > > > #if defined(USE_OZONE)
> > > > ...
> > > > #endif
> > > > gfx::Point point_in_screen = ..
> > > > 
> > > > 
> > > > I think this is easier to follow.
> > > 
> > > Done

[dnicoara, 2014-09-19 13:56:51]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-09-19 13:57:19]

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

[commit-bot: I haz the power, 2014-09-19 14:27:07]

Committed patchset #4 (id:60001) as ee268e25492e64bfd453b283b8bcb79f78095f02

[commit-bot: I haz the power, 2014-09-19 14:27:43]

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