Fixes rounding error when calculating MOVE event

Fixes rounding error when calculating MOVE event

In general, it is good practice to compare floats using an Epsilon.
In particular, a mouse location_f() could differ between the
MOUSE_PRESSED and MOUSE_RELEASED events. At MOUSE_RELEASED, it will have
a targeter() already cached, while at MOUSE_PRESSED, it will have to
calculate it passing through all  windows, and that could generate
rounding error.

This patch fixes, that situation. It could be triggered when one of the
window->layer()->bounds()'s origin is at a value bigger than 9000

BUG=723902
R=reveman@chromium.org

Review-Url: https://codereview.chromium.org/2894503002
Cr-Commit-Position: refs/heads/master@{#474854}
Committed: https://chromium.googlesource.com/chromium/src/+/0092647e4e34a209afc7f8715a9512c26a787457

[Luis Héctor Chávez, 2017-05-17 23:57:10]

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

[Luis Héctor Chávez, 2017-05-17 23:57:11]

drive-by

https://codereview.chromium.org/2894503002/diff/1/components/exo/pointer.cc
File components/exo/pointer.cc (right):

https://codereview.chromium.org/2894503002/diff/1/components/exo/pointer.cc#n...
components/exo/pointer.cc:51: static const float epsilon_scale = 0.0005f;
nit: constants are usually declared at the very top of the file and with the
convention kMyConstantName (see kLargeCursorScale in L35).

Also, I wonder if it's worth exposing
https://cs.chromium.org/chromium/src/ui/gfx/geometry/quad_f.cc?type=cs&q=eps+...
and addint the epsilon value as template parameter.

[Yusuke Sato, 2017-05-18 00:23:39]

The CQ bit was checked by yusukes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 00:24:53]

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-05-18 02:21:54]

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

[commit-bot: I haz the power, 2017-05-18 02:21:55]

Dry run: This issue passed the CQ dry run.

[Yusuke Sato, 2017-05-18 02:31:39]

Description was changed from

==========
Fixes rounding error when calculating MOVE event

In general, it is good practice to compare floats using an Epsilon.
In particular, a mouse location_f() could differ between the
MOUSE_PRESSED and MOUSE_RELEASED events. At MOUSE_RELEASED, it will have
a targeter() already cached, while at MOUSE_PRESSED, it will have to
calculate it passing through all  windows, and that could generate
rounding error.

This patch fixes, that situation. It could be triggered when one of the
window->layer()->bounds()'s origin is at a value bigger than 9000

BUG=723902
R=reveman@chromium.org
==========

to

==========
Fixes rounding error when calculating MOVE event

In general, it is good practice to compare floats using an Epsilon.
In particular, a mouse location_f() could differ between the
MOUSE_PRESSED and MOUSE_RELEASED events. At MOUSE_RELEASED, it will have
a targeter() already cached, while at MOUSE_PRESSED, it will have to
calculate it passing through all  windows, and that could generate
rounding error.

This patch fixes, that situation. It could be triggered when one of the
window->layer()->bounds()'s origin is at a value bigger than 9000

BUG=723902
R=reveman@chromium.org
==========

[Yusuke Sato, 2017-05-18 02:31:39]

yusukes@chromium.org changed reviewers:
- yusukes@chromium.org

[reveman, 2017-05-18 04:21:10]

https://codereview.chromium.org/2894503002/diff/1/components/exo/pointer.cc
File components/exo/pointer.cc (right):

https://codereview.chromium.org/2894503002/diff/1/components/exo/pointer.cc#n...
components/exo/pointer.cc:51: static const float epsilon_scale = 0.0005f;
On 2017/05/17 at 23:57:11, Luis Héctor Chávez wrote:
> nit: constants are usually declared at the very top of the file and with the
convention kMyConstantName (see kLargeCursorScale in L35).

Yes, kLocatedEventEpsilon above is better. Also, maybe write this as 1.0f /
2000.0f;

> 
> Also, I wonder if it's worth exposing
https://cs.chromium.org/chromium/src/ui/gfx/geometry/quad_f.cc?type=cs&q=eps+...
and addint the epsilon value as template parameter.

Sg, keeping in this file is fine with me as well.

[Luis Héctor Chávez, 2017-05-22 19:42:52]

one final nit

https://codereview.chromium.org/2894503002/diff/20001/components/exo/pointer.cc
File components/exo/pointer.cc (right):

https://codereview.chromium.org/2894503002/diff/20001/components/exo/pointer....
components/exo/pointer.cc:38: static const float kLocatedEventEpsilon = 1.0f /
2000.0f;
nit: 'static' is unnecessary here since you're already in the anonymous
namespace.

[riq, 2017-05-22 19:52:09]

On 2017/05/22 19:42:52, Luis Héctor Chávez wrote:
> one final nit
> 
>
https://codereview.chromium.org/2894503002/diff/20001/components/exo/pointer.cc
> File components/exo/pointer.cc (right):
> 
>
https://codereview.chromium.org/2894503002/diff/20001/components/exo/pointer....
> components/exo/pointer.cc:38: static const float kLocatedEventEpsilon = 1.0f /
> 2000.0f;
> nit: 'static' is unnecessary here since you're already in the anonymous
> namespace.

thanks. fixed.

[riq, 2017-05-22 19:52:33]

> Yes, kLocatedEventEpsilon above is better. Also, maybe write this as 1.0f /
> 2000.0f;


thanks. fixed.

[riq, 2017-05-22 19:54:02]

thanks, fixed all the suggested changes.

[reveman, 2017-05-22 20:12:28]

https://codereview.chromium.org/2894503002/diff/40001/components/exo/pointer.cc
File components/exo/pointer.cc (right):

https://codereview.chromium.org/2894503002/diff/40001/components/exo/pointer....
components/exo/pointer.cc:57: kLocatedEventEpsilon;
how about:

gfx::Vector2dF offset = event->location_f() - location;
return offset.LengthSquared() < kLocatedEventEpsilon;

and adjust the Epsilon constant accordingly? I think that might be a bit easier
to read.

[riq, 2017-05-24 00:34:21]

Patchset #5 (id:80001) has been deleted

[riq, 2017-05-24 00:35:29]

thanks. using `vector.lenghtSquared()` as suggested.

[reveman, 2017-05-24 13:14:01]

lgtm with nit

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

https://codereview.chromium.org/2894503002/diff/60001/components/exo/pointer....
components/exo/pointer.cc:7: #include <utility>
nit: what is this for? please remove if not needed.

https://codereview.chromium.org/2894503002/diff/60001/components/exo/pointer....
components/exo/pointer.cc:55: return offset.LengthSquared() <
optional nit:

const double kLocatedEventEpsilonSquared = 2 * kLocatedEventEpsilon *
kLocatedEventEpsilon;
return offset.LengthSquared() < kLocatedEventEpsilonSquared;

makes it a bit more clear what is computed at compile time vs runtime.

[riq, 2017-05-24 16:59:55]

thanks. fixed. using `KLocatedEventEpsilonSquared`.

I'm still using the `2 *` in the calculation and not in the constant. Assuming
the compiler is smart enough to pre-compute that value at compile time (and not
runtime).

I can move it back to the definition of the constant if needed.

I need the `2 *` since the now we are using `distance()`. Before it was not
needed since each coordinate was evaluated independently.
Using distance changes the logic is a tiny bit, but should not affect the
rounding error detection.

[reveman, 2017-05-25 11:12:01]

still lgtm

https://codereview.chromium.org/2894503002/diff/100001/components/exo/pointer.cc
File components/exo/pointer.cc (right):

https://codereview.chromium.org/2894503002/diff/100001/components/exo/pointer...
components/exo/pointer.cc:38: const double kLocatedEventEpsilonSquared = (1.0 /
2000.0) * (1.0 / 2000.0);
this is fine but maybe "= 1.0 / 4000000.0" or " = 1.0 / (2000.0 * 2000.0)"
instead

[riq, 2017-05-25 23:51:14]

thanks. I've just sent a final CL with the `(2000 * 2000)` suggestion.
I'll commit it.

[riq, 2017-05-25 23:51:53]

The CQ bit was checked by ricardoq@chromium.org

[riq, 2017-05-25 23:51:54]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2894503002/#ps120001
(title: "Fixes rounding error when calculating MOVE event")

[commit-bot: I haz the power, 2017-05-25 23:53:00]

CQ is trying da patch.

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

[reveman, 2017-05-25 23:59:12]

https://codereview.chromium.org/2894503002/diff/120001/components/exo/pointer.cc
File components/exo/pointer.cc (right):

https://codereview.chromium.org/2894503002/diff/120001/components/exo/pointer...
components/exo/pointer.cc:7: #include <utility>
why was this needed?

[riq, 2017-05-26 00:27:38]

On 2017/05/25 23:59:12, reveman wrote:
>
https://codereview.chromium.org/2894503002/diff/120001/components/exo/pointer.cc
> File components/exo/pointer.cc (right):
> 
>
https://codereview.chromium.org/2894503002/diff/120001/components/exo/pointer...
> components/exo/pointer.cc:7: #include <utility>
> why was this needed?

`git cl lint` say that it was missing. perhaps some forgot to add it in a
previous commit:

$ git cl lint .
Skipping file .
components/exo/pointer.cc:365:  Add #include <utility> for move 
[build/include_what_you_use] [4]
Done processing components/exo/pointer.cc
Total errors found: 1

[reveman, 2017-05-26 00:34:19]

Ok, Thanks for explaining and fixing

On May 25, 2017 8:27 PM, <ricardoq@chromium.org> wrote:

> On 2017/05/25 23:59:12, reveman wrote:
> >
> https://codereview.chromium.org/2894503002/diff/120001/
> components/exo/pointer.cc
> > File components/exo/pointer.cc (right):
> >
> >
> https://codereview.chromium.org/2894503002/diff/120001/
> components/exo/pointer.cc#newcode7
> > components/exo/pointer.cc:7: #include <utility>
> > why was this needed?
>
> `git cl lint` say that it was missing. perhaps some forgot to add it in a
> previous commit:
>
> $ git cl lint .
> Skipping file .
> components/exo/pointer.cc:365: Add #include <utility> for move
> [build/include_what_you_use] [4]
> Done processing components/exo/pointer.cc
> Total errors found: 1
>
>
> https://codereview.chromium.org/2894503002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-05-26 00:51:54]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495756313765000,
"parent_rev": "0563f6fc4ee16a1259bc259d5ec7d7bca1495b50", "commit_rev":
"0092647e4e34a209afc7f8715a9512c26a787457"}

[commit-bot: I haz the power, 2017-05-26 00:52:04]

Description was changed from

==========
Fixes rounding error when calculating MOVE event

In general, it is good practice to compare floats using an Epsilon.
In particular, a mouse location_f() could differ between the
MOUSE_PRESSED and MOUSE_RELEASED events. At MOUSE_RELEASED, it will have
a targeter() already cached, while at MOUSE_PRESSED, it will have to
calculate it passing through all  windows, and that could generate
rounding error.

This patch fixes, that situation. It could be triggered when one of the
window->layer()->bounds()'s origin is at a value bigger than 9000

BUG=723902
R=reveman@chromium.org
==========

to

==========
Fixes rounding error when calculating MOVE event

In general, it is good practice to compare floats using an Epsilon.
In particular, a mouse location_f() could differ between the
MOUSE_PRESSED and MOUSE_RELEASED events. At MOUSE_RELEASED, it will have
a targeter() already cached, while at MOUSE_PRESSED, it will have to
calculate it passing through all  windows, and that could generate
rounding error.

This patch fixes, that situation. It could be triggered when one of the
window->layer()->bounds()'s origin is at a value bigger than 9000

BUG=723902
R=reveman@chromium.org

Review-Url: https://codereview.chromium.org/2894503002
Cr-Commit-Position: refs/heads/master@{#474854}
Committed:
https://chromium.googlesource.com/chromium/src/+/0092647e4e34a209afc7f8715a95...
==========

[commit-bot: I haz the power, 2017-05-26 00:52:08]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/0092647e4e34a209afc7f8715a95...