Handle floating point coordinates from ozone to exosphere

Handle floating point coordinates from ozone to exosphere

Most touch coordinate handling within chrome is done with gfx:Points.
Rounding to points works well for most cases, but really degrades
user experience for hand writing with an on-screen stylus.
This change will add variants to a couple of coordinate transformation
methods that will use gfx::PointF instead of gfx::Point.
This allows touch coordinates to be passed from ozone to exosphere in
sub-pixel accuracy, allowing those coordinates to be used in arc++
note taking apps.

BUG=681235
TEST=Test with arc++ drawing apps without smoothing. Lines should
be smooth without distinct steps.

Review-Url: https://codereview.chromium.org/2648583003
Cr-Commit-Position: refs/heads/master@{#445510}
Committed: https://chromium.googlesource.com/chromium/src/+/951e1c2407a8055588e51f2f7768f034c32b3e06

[denniskempin, 2017-01-19 23:55:53]

The CQ bit was checked by denniskempin@google.com to run a CQ dry run

[denniskempin, 2017-01-19 23:56:50]

denniskempin@google.com changed reviewers:
+ reveman@chromium.org, tdresser@chromium.org

[denniskempin, 2017-01-20 02:06:48]

denniskempin@google.com changed reviewers:
+ sadrul@chromium.org

[denniskempin, 2017-01-20 02:06:48]

Sadrul, what do you think about this? It's a first step towards processing touch
coordinates in floating point. This is pretty critical for stylus events on
Kevin for a good note taking experience.

[tdresser, 2017-01-20 16:07:48]

https://codereview.chromium.org/2648583003/diff/20001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2648583003/diff/20001/ui/aura/window.cc#newco...
ui/aura/window.cc:447: gfx::Point floored = gfx::ToFlooredPoint(*point);
Do we need to floor in this case?

[denniskempin, 2017-01-20 18:26:33]

https://codereview.chromium.org/2648583003/diff/20001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2648583003/diff/20001/ui/aura/window.cc#newco...
ui/aura/window.cc:447: gfx::Point floored = gfx::ToFlooredPoint(*point);
On 2017/01/20 16:07:48, tdresser wrote:
> Do we need to floor in this case?

I am flooring here to be able to pass a Point into the ScreenPositionClient
methods. I was trying to avoid updating the ScreenPositionClient interface to
have PointF support as well, since it has a whole bunch of implementations. 
But I can try it to see how far reaching that change would be.

[denniskempin, 2017-01-20 19:30:26]

The CQ bit was checked by denniskempin@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 19:31:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 20:05:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 20:05:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[denniskempin, 2017-01-20 20:49:29]

The CQ bit was checked by denniskempin@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 20:49:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[denniskempin, 2017-01-20 20:50:19]

denniskempin@google.com changed reviewers:
+ oshima@chromium.org

[denniskempin, 2017-01-20 20:50:19]

Hi Oshima! Could you have a look at the changes in ash?

[sky, 2017-01-20 21:26:37]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2017-01-20 21:26:37]

This makes me sad. It's a work around for using DIPs. *SIGH*

Please add test coverage.

[reveman, 2017-01-20 21:38:24]

components/exo lgtm

quick question, why not do the same in exo code for pointer events?

[oshima, 2017-01-20 22:11:55]

ash lgtm

https://codereview.chromium.org/2648583003/diff/60001/components/exo/touch.cc
File components/exo/touch.cc (right):

https://codereview.chromium.org/2648583003/diff/60001/components/exo/touch.cc...
components/exo/touch.cc:82: &location);
If event system can pass through root_location_f, then it should be possible to
convert it to the focus's coordinate using layer's conversion, because the
target and focus should always be the same.

I'm not sure why/wehre that root location needs to be converted during dispatch
process. sadrul@ may know a way to make it work tho.

https://codereview.chromium.org/2648583003/diff/60001/ui/aura/client/screen_p...
File ui/aura/client/screen_position_client.h (right):

https://codereview.chromium.org/2648583003/diff/60001/ui/aura/client/screen_p...
ui/aura/client/screen_position_client.h:32: virtual void
ConvertPointToScreenF(const Window* window,
I have a slight preference for ConvertPointFToScreen. I'll leave it to the owner
though.

https://codereview.chromium.org/2648583003/diff/60001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2648583003/diff/60001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:550: }
new line

[commit-bot: I haz the power, 2017-01-20 22:12:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 22:12:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sadrul, 2017-01-20 22:35:19]

https://codereview.chromium.org/2648583003/diff/60001/components/exo/touch.cc
File components/exo/touch.cc (right):

https://codereview.chromium.org/2648583003/diff/60001/components/exo/touch.cc...
components/exo/touch.cc:82: &location);
On 2017/01/20 22:11:54, oshima wrote:
> If event system can pass through root_location_f, then it should be possible
to
> convert it to the focus's coordinate using layer's conversion, because the
> target and focus should always be the same.
> 
> I'm not sure why/wehre that root location needs to be converted during
dispatch
> process. sadrul@ may know a way to make it work tho.

|event| should have the correct root-location here. |root_location| of the event
gets transformed for high-DPI (and rotation), so the root-location that you get
here should give you the root-location you expect, and converting that
root-location from |focus_->window()->GetRootWindow()| to |focus_->window()|
should give you the correct location-f.

If that works, does that mean you don't need to change the rest of the code
outside of exo?

[denniskempin, 2017-01-20 23:22:19]

https://codereview.chromium.org/2648583003/diff/60001/components/exo/touch.cc
File components/exo/touch.cc (right):

https://codereview.chromium.org/2648583003/diff/60001/components/exo/touch.cc...
components/exo/touch.cc:82: &location);
On 2017/01/20 22:35:18, sadrul wrote:
> On 2017/01/20 22:11:54, oshima wrote:
> > If event system can pass through root_location_f, then it should be possible
> to
> > convert it to the focus's coordinate using layer's conversion, because the
> > target and focus should always be the same.
> > 
> > I'm not sure why/wehre that root location needs to be converted during
> dispatch
> > process. sadrul@ may know a way to make it work tho.
> 
> |event| should have the correct root-location here. |root_location| of the
event
> gets transformed for high-DPI (and rotation), so the root-location that you
get
> here should give you the root-location you expect, and converting that
> root-location from |focus_->window()->GetRootWindow()| to |focus_->window()|
> should give you the correct location-f.
> 
> If that works, does that mean you don't need to change the rest of the code
> outside of exo?

I am trying that right now, it would be great if that works! We would still need
the EventLocationFromNativeF method to not round when creating the TouchEvent,
but none of the ConvertPointToTarget methods.

[denniskempin, 2017-01-21 00:35:48]

It works!! Thanks sadrul and oshima!

I saw some code in the dispatching path that would re-calculate root_location_
from location_, but I noticed that's a code path that is only being used when
re-targeting an event to a different root. 

Either way, sadrul, sky please have a look at the updated CL. All that's left to
change on the Chrome side is the addition of ui::EventLocationFromNativeF. 

I have extended some test cases to make sure EventLocationFromNative and
EventLocationFromNativeF return the same coordinates (when floored). I couldn't
find any test cases that cover EventLocationFromNative for ozone and windows. 

reveman, does your lgtm still stand after the updated exo code?

[denniskempin, 2017-01-21 00:36:17]

The CQ bit was checked by denniskempin@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-21 00:36:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 02:06:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-21 02:06:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2017-01-23 16:08:00]

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

https://codereview.chromium.org/2648583003/diff/100001/ui/events/event_utils....
ui/events/event_utils.h:71: EVENTS_EXPORT gfx::Point EventLocationFromNative(
Do we still call the non-floating point version?

[oshima, 2017-01-23 16:34:32]

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

https://codereview.chromium.org/2648583003/diff/100001/ui/events/event_utils....
ui/events/event_utils.h:70: // TODO(tdresser): Return gfx::PointF here. See
crbug.com/337827.
We should be able to remove old one (we may want to do it in separate CL if this
has to be merged). Can you add this one to BUG too?

[sky, 2017-01-23 17:04:06]

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

https://codereview.chromium.org/2648583003/diff/100001/ui/events/event_utils....
ui/events/event_utils.h:70: // TODO(tdresser): Return gfx::PointF here. See
crbug.com/337827.
On 2017/01/23 16:34:32, oshima wrote:
> We should be able to remove old one (we may want to do it in separate CL if
this
> has to be merged). Can you add this one to BUG too?

If the only caller is from the constructor that was just changed, it should be
easy to remove it now.

[reveman, 2017-01-23 17:09:03]

components/exo lgtm still but I'm still curious to why we're not updating
exo::Pointer in the same way as part of this change? What makes touch events
unique in this regard?

[denniskempin, 2017-01-23 20:36:42]

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

https://codereview.chromium.org/2648583003/diff/100001/ui/events/event_utils....
ui/events/event_utils.h:71: EVENTS_EXPORT gfx::Point EventLocationFromNative(
The method is called in a couple of locations besides that Constructor and
unittests. See:
https://cs.chromium.org/search/?q=EventLocationFromNative&sq=package:chromium...

They are not many, but since we need to merge this into M56 (now stable or about
to be stable) I would like to keep the impact as low as possible. What do you
think?

[denniskempin, 2017-01-23 21:04:06]

The CQ bit was checked by denniskempin@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 21:04:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-01-23 21:42:22]

Dennis and myself IMd offline. He said he would cleanup the unnecessary calls
after this lands. LGTM

[denniskempin, 2017-01-23 21:57:25]

The CQ bit was unchecked by denniskempin@google.com

[denniskempin, 2017-01-23 21:57:31]

The CQ bit was checked by denniskempin@google.com

[denniskempin, 2017-01-23 21:57:31]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2648583003/#ps120001
(title: "fixed wayland unittest")

[commit-bot: I haz the power, 2017-01-23 21:58:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 22:17:16]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485208651327780,
"parent_rev": "9079de0121eb747464e490e29fd2444dde5f0847", "commit_rev":
"951e1c2407a8055588e51f2f7768f034c32b3e06"}

[commit-bot: I haz the power, 2017-01-23 22:17:55]

Description was changed from

==========
Handle floating point coordinates from ozone to exosphere

Most touch coordinate handling within chrome is done with gfx:Points.
Rounding to points works well for most cases, but really degrades
user experience for hand writing with an on-screen stylus.
This change will add variants to a couple of coordinate transformation
methods that will use gfx::PointF instead of gfx::Point.
This allows touch coordinates to be passed from ozone to exosphere in
sub-pixel accuracy, allowing those coordinates to be used in arc++
note taking apps.

BUG=681235
TEST=Test with arc++ drawing apps without smoothing. Lines should
be smooth without distinct steps.
==========

to

==========
Handle floating point coordinates from ozone to exosphere

Most touch coordinate handling within chrome is done with gfx:Points.
Rounding to points works well for most cases, but really degrades
user experience for hand writing with an on-screen stylus.
This change will add variants to a couple of coordinate transformation
methods that will use gfx::PointF instead of gfx::Point.
This allows touch coordinates to be passed from ozone to exosphere in
sub-pixel accuracy, allowing those coordinates to be used in arc++
note taking apps.

BUG=681235
TEST=Test with arc++ drawing apps without smoothing. Lines should
be smooth without distinct steps.

Review-Url: https://codereview.chromium.org/2648583003
Cr-Commit-Position: refs/heads/master@{#445510}
Committed:
https://chromium.googlesource.com/chromium/src/+/951e1c2407a8055588e51f2f7768...
==========

[commit-bot: I haz the power, 2017-01-23 22:17:57]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/951e1c2407a8055588e51f2f7768...