Chrome requires WebTouchPoint to store WebFloatPoint, instead of WebPoint.

Chrome requires WebTouchPoint to store WebFloatPoint, instead of WebPoint.

This is the third of three patches converting WebTouchPoint to use
WebFloatPoint instead of WebPoint. This cleans up some temporary
casts to WebPoint in chromium.

The first patch (https://codereview.chromium.org/140973005) allows
chromium to accept WebFloatPoint (it just casts all the floats to
ints).

The second patch was a blink modification to store WebFloatPoint
(https://codereview.chromium.org/149053002/).

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

[tdresser, 2014-03-10 21:17:56]

jdduke, can you take a look at 
- content/browser/renderer_host/input/*
- content/common/input/*
- content/renderer/input/*

Specifically, can you make sure the touch slop change looks correct?

[jdduke (slow), 2014-03-10 22:03:22]

https://codereview.chromium.org/148453012/diff/80001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/148453012/diff/80001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:179:
touch_sequence_start_position_ =
Hmm, does this fit on one line now?

https://codereview.chromium.org/148453012/diff/80001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:194: if ((position -
touch_sequence_start_position_).LengthSquared() >
So, I'm wondering if we'll run into floating point issues here... Sadly, I'm not
sure there's an easy way around that other than converting back to pixels and
doing the test on properly rounded integral values.  It might be worth adding a
short test that checks the inclusive slop region using scaling factors of 1.5
and .75 on coordinates larger than, say, 1000 (where coords outside the region
by some small epsilon are not suppressed).

https://codereview.chromium.org/148453012/diff/80001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/148453012/diff/80001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:1425: // Tests
that TouchMove's are not dropped due incorrect handling of DPI scaling.
Nit: "due to"

https://codereview.chromium.org/148453012/diff/80001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/148453012/diff/80001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:232:
gfx::ToFlooredPoint(touch_event.touches[i].position))) {
What about changing InputHandler::HaveTouchEventHandlersAt() to take a
gfx::PointF? Or would that be a follow-up? Looking at LayerTreeHostImpl, it
should be able to operate on a gfx::PointF without any meaningful change other
than the method signature.

[tdresser, 2014-03-12 13:54:39]

https://codereview.chromium.org/148453012/diff/80001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/148453012/diff/80001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:179:
touch_sequence_start_position_ =
On 2014/03/10 22:03:23, jdduke wrote:
> Hmm, does this fit on one line now?

Yup, done.

https://codereview.chromium.org/148453012/diff/80001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:194: if ((position -
touch_sequence_start_position_).LengthSquared() >
On 2014/03/10 22:03:23, jdduke wrote:
> So, I'm wondering if we'll run into floating point issues here... Sadly, I'm
not
> sure there's an easy way around that other than converting back to pixels and
> doing the test on properly rounded integral values.  It might be worth adding
a
> short test that checks the inclusive slop region using scaling factors of 1.5
> and .75 on coordinates larger than, say, 1000 (where coords outside the region
> by some small epsilon are not suppressed).

That's unfortunate. Good call though. Rounding broke one previous test:
TouchEventQueueTest.TouchMoveSuppressionWithinSlopRegion. I think the change to
fix it is reasonable, but can you double check it? Rounding means that in some
cases you need to exceed the slop more than was necessary previously in order to
stop suppressing touch moves.

https://codereview.chromium.org/148453012/diff/80001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/148453012/diff/80001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:1425: // Tests
that TouchMove's are not dropped due incorrect handling of DPI scaling.
On 2014/03/10 22:03:23, jdduke wrote:
> Nit: "due to"

Done.

https://codereview.chromium.org/148453012/diff/80001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/148453012/diff/80001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:232:
gfx::ToFlooredPoint(touch_event.touches[i].position))) {
On 2014/03/10 22:03:23, jdduke wrote:
> What about changing InputHandler::HaveTouchEventHandlersAt() to take a
> gfx::PointF? Or would that be a follow-up? Looking at LayerTreeHostImpl, it
> should be able to operate on a gfx::PointF without any meaningful change other
> than the method signature.

I did that in patch set #4. It felt like it involved changing a bunch of code
with no clear benefit. Why do you feel it would be preferable? Just to reduce
the number of ToFlooredPoint calls?

[jdduke (slow), 2014-03-12 15:27:26]

https://codereview.chromium.org/148453012/diff/100001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/148453012/diff/100001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:157:
gfx::Screen::GetNativeScreen()
Sorry, I didn't mean to suggest we have to fix this now, and I'm not sure we
want to start caching the DPI scale here. We should probably just remain
conservative in our touchmove suppression (i.e., we never want to suppress a
touchmove that has exceeded the threshold, but it's not the end of the world if
we allow through a touchmove just inside the slop threshold).  I have an
outstanding bug crbug.com/344604 to bundle the DPI scale with
Web{Touch,Gesture}Events, allowing code anywhere to recompute the original
integral coords, without risk of using a stale DPI scaling factor. Maybe you can
just put a reference to that bug in our current slop suppression code?

Another thought is that we accompany the TouchMove with a bit indicating that
this TouchMove would start a scroll sequence (data readily available with the
eager GR). Then we're fully decoupled from the slop logic, but I don't think we
need to implement that now.

[jdduke (slow), 2014-03-19 19:50:05]

On 2014/03/12 15:27:26, jdduke wrote:
>
https://codereview.chromium.org/148453012/diff/100001/content/browser/rendere...
> File content/browser/renderer_host/input/input_router_impl.cc (right):
> 
>
https://codereview.chromium.org/148453012/diff/100001/content/browser/rendere...
> content/browser/renderer_host/input/input_router_impl.cc:157:
> gfx::Screen::GetNativeScreen()
> Sorry, I didn't mean to suggest we have to fix this now, and I'm not sure we
> want to start caching the DPI scale here. We should probably just remain
> conservative in our touchmove suppression (i.e., we never want to suppress a
> touchmove that has exceeded the threshold, but it's not the end of the world
if
> we allow through a touchmove just inside the slop threshold).  I have an
> outstanding bug crbug.com/344604 to bundle the DPI scale with
> Web{Touch,Gesture}Events, allowing code anywhere to recompute the original
> integral coords, without risk of using a stale DPI scaling factor. Maybe you
can
> just put a reference to that bug in our current slop suppression code?
> 
> Another thought is that we accompany the TouchMove with a bit indicating that
> this TouchMove would start a scroll sequence (data readily available with the
> eager GR). Then we're fully decoupled from the slop logic, but I don't think
we
> need to implement that now.

Hopefully you weren't waiting for further comment from me here, sorry! After
further thought I definitely don't think we need to address the slop precision
issue here.  Your change to touch_event_queue.cc in patch set 3 looked good.

[tdresser, 2014-03-20 11:43:04]

jdduke@, does this seem like a reasonable state to leave things for now?

jochen@, can you take a look at:
content/browser/renderer_host/ui_events_helper.cc
content/renderer/render_widget.cc
content/shell/renderer/test_runner/*

[jdduke (slow), 2014-03-20 12:11:33]

content/browser/renderer_host/input, content/common/input and
content/renderer/input lgtm with a couple questions.

https://codereview.chromium.org/148453012/diff/120001/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/148453012/diff/120001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:195:
slop_suppression_length_dips_squared_)
Hmm, does clang-format not indent the |slop_suppression_length_dips_squared_| as
the second line of the if? If not, this is fine.

https://codereview.chromium.org/148453012/diff/120001/content/renderer/input/...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/148453012/diff/120001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:235:
gfx::ToFlooredPoint(touch_event.touches[i].position))) {
Hmm, this may be a small change in behavior.  Previously, the coords were
truncated, but now they're being floored, no?  Can we just use gfx::Point()
constructor here, and eventually we should update the method to take a
gfx::PointF.

[tdresser, 2014-03-20 14:09:14]

https://codereview.chromium.org/148453012/diff/120001/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/148453012/diff/120001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:195:
slop_suppression_length_dips_squared_)
On 2014/03/20 12:11:34, jdduke wrote:
> Hmm, does clang-format not indent the |slop_suppression_length_dips_squared_|
as
> the second line of the if? If not, this is fine.

Clang-format agrees with what's there.

https://codereview.chromium.org/148453012/diff/120001/content/renderer/input/...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/148453012/diff/120001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:235:
gfx::ToFlooredPoint(touch_event.touches[i].position))) {
On 2014/03/20 12:11:34, jdduke wrote:
> Hmm, this may be a small change in behavior.  Previously, the coords were
> truncated, but now they're being floored, no?  Can we just use gfx::Point()
> constructor here, and eventually we should update the method to take a
> gfx::PointF.

Done. 

I'm not convinced it really makes sense to make HaveTouchEventHandlersAt take a
gfx::PointF. Patch #4 did this, but I couldn't see much benefit. Why do you
think it would be preferable?

[jdduke (slow), 2014-03-20 14:15:35]

On 2014/03/20 14:09:14, tdresser wrote:
>
https://codereview.chromium.org/148453012/diff/120001/content/browser/rendere...
> File content/browser/renderer_host/input/touch_event_queue.cc (right):
> 
>
https://codereview.chromium.org/148453012/diff/120001/content/browser/rendere...
> content/browser/renderer_host/input/touch_event_queue.cc:195:
> slop_suppression_length_dips_squared_)
> On 2014/03/20 12:11:34, jdduke wrote:
> > Hmm, does clang-format not indent the
|slop_suppression_length_dips_squared_|
> as
> > the second line of the if? If not, this is fine.
> 
> Clang-format agrees with what's there.
> 
>
https://codereview.chromium.org/148453012/diff/120001/content/renderer/input/...
> File content/renderer/input/input_handler_proxy.cc (right):
> 
>
https://codereview.chromium.org/148453012/diff/120001/content/renderer/input/...
> content/renderer/input/input_handler_proxy.cc:235:
> gfx::ToFlooredPoint(touch_event.touches[i].position))) {
> On 2014/03/20 12:11:34, jdduke wrote:
> > Hmm, this may be a small change in behavior.  Previously, the coords were
> > truncated, but now they're being floored, no?  Can we just use gfx::Point()
> > constructor here, and eventually we should update the method to take a
> > gfx::PointF.
> 
> Done. 
> 
> I'm not convinced it really makes sense to make HaveTouchEventHandlersAt take
a
> gfx::PointF. Patch #4 did this, but I couldn't see much benefit. Why do you
> think it would be preferable?

If you look at LayerTreeHostImpl::HaveTouchEventHandlersAt, the |viewport_point|
is scaled back to device pixels, so we're losing precision by passing integer
coords.

Again, I don't think it's necessary to change the interface now, but I do think
we should do so in a follow-up patch.

[tdresser, 2014-03-20 14:19:53]

On 2014/03/20 14:15:35, jdduke wrote:
> On 2014/03/20 14:09:14, tdresser wrote:
> >
>
https://codereview.chromium.org/148453012/diff/120001/content/browser/rendere...
> > File content/browser/renderer_host/input/touch_event_queue.cc (right):
> > 
> >
>
https://codereview.chromium.org/148453012/diff/120001/content/browser/rendere...
> > content/browser/renderer_host/input/touch_event_queue.cc:195:
> > slop_suppression_length_dips_squared_)
> > On 2014/03/20 12:11:34, jdduke wrote:
> > > Hmm, does clang-format not indent the
> |slop_suppression_length_dips_squared_|
> > as
> > > the second line of the if? If not, this is fine.
> > 
> > Clang-format agrees with what's there.
> > 
> >
>
https://codereview.chromium.org/148453012/diff/120001/content/renderer/input/...
> > File content/renderer/input/input_handler_proxy.cc (right):
> > 
> >
>
https://codereview.chromium.org/148453012/diff/120001/content/renderer/input/...
> > content/renderer/input/input_handler_proxy.cc:235:
> > gfx::ToFlooredPoint(touch_event.touches[i].position))) {
> > On 2014/03/20 12:11:34, jdduke wrote:
> > > Hmm, this may be a small change in behavior.  Previously, the coords were
> > > truncated, but now they're being floored, no?  Can we just use
gfx::Point()
> > > constructor here, and eventually we should update the method to take a
> > > gfx::PointF.
> > 
> > Done. 
> > 
> > I'm not convinced it really makes sense to make HaveTouchEventHandlersAt
take
> a
> > gfx::PointF. Patch #4 did this, but I couldn't see much benefit. Why do you
> > think it would be preferable?
> 
> If you look at LayerTreeHostImpl::HaveTouchEventHandlersAt, the
|viewport_point|
> is scaled back to device pixels, so we're losing precision by passing integer
> coords.
> 
> Again, I don't think it's necessary to change the interface now, but I do
think
> we should do so in a follow-up patch.

Ah, good point, I'm convinced. Filed as issue 354436.

[jochen (gone - plz use gerrit), 2014-03-20 15:15:17]

lgtm

[tdresser, 2014-03-20 15:18:21]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-03-20 15:19:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/148453012/140001

[commit-bot: I haz the power, 2014-03-20 18:27:41]

Change committed as 258357