Make touch-based synthetic gesture take touch slop into account.

Make touch-based synthetic gesture take touch slop into account.

Touchscreen devices typically have a 'touch slop', i.e. a certain distance a
pointer has to move before it is considered moving. The touch slop is subtracted
from the distance a pointer moves. So if the touch slop is 10 pixels and a
pointer moves by 100 pixels on the screen, the resulting move is registered as
having covered only 90 pixels.

This patch adds the 'GetTouchSlopInDips' to SyntheticGestureTargets. This value
is used by the gestures to increase the distances covered by touch pointers. For
example, to scroll 100 pixels with a touch slop of 10 pixels, the pointer will
cover 110 pixels.

Touch-based synthetic gestures are updated to take the touch slop into account.

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

[Dominik Grewe, 2013-12-02 12:03:35]

PTAL, thanks!

https://codereview.chromium.org/95153002/diff/80001/content/common/android/vi...
File content/common/android/view_configuration.cc (right):

https://codereview.chromium.org/95153002/diff/80001/content/common/android/vi...
content/common/android/view_configuration.cc:57: return
Java_ViewConfiguration_getTouchSlop(env);
This method is actually deprecated (and so are the "non-scaled" ones above).

[jdduke (slow), 2013-12-02 15:35:32]

Can you separate out the pinch gesture refactoring into a separate patch?
Thanks.

[jdduke (slow), 2013-12-02 15:51:39]

https://codereview.chromium.org/95153002/diff/80001/content/browser/renderer_...
File content/browser/renderer_host/input/synthetic_gesture_target_android.cc
(right):

https://codereview.chromium.org/95153002/diff/80001/content/browser/renderer_...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:94:
ViewConfiguration::GetTouchSlopInPixels() / view->GetDeviceScaleFactor();
You can use the following to save yourself some trouble:

gfx::Screen::GetNativeScreen()->GetPrimaryDisplay().device_scale_factor();

https://codereview.chromium.org/95153002/diff/80001/content/common/android/vi...
File content/common/android/view_configuration.cc (right):

https://codereview.chromium.org/95153002/diff/80001/content/common/android/vi...
content/common/android/view_configuration.cc:57: return
Java_ViewConfiguration_getTouchSlop(env);
On 2013/12/02 12:03:36, Dominik Grewe wrote:
> This method is actually deprecated (and so are the "non-scaled" ones above).

Let's remove the GetTouchSlopInDips() then so people don't start using it.  

I don't believe the fling velocity-related methods are yet in use, but we should
probably remove those as well (separately, I can do that).  That or re-implement
them in terms of device scale factor and the scaled queries.

[Dominik Grewe, 2013-12-02 16:18:53]

Done. PTAL.

https://codereview.chromium.org/95153002/diff/80001/content/browser/renderer_...
File content/browser/renderer_host/input/synthetic_gesture_target_android.cc
(right):

https://codereview.chromium.org/95153002/diff/80001/content/browser/renderer_...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:94:
ViewConfiguration::GetTouchSlopInPixels() / view->GetDeviceScaleFactor();
On 2013/12/02 15:51:40, jdduke wrote:
> You can use the following to save yourself some trouble:
> 
> gfx::Screen::GetNativeScreen()->GetPrimaryDisplay().device_scale_factor();

Nice! Wasn't aware of that.

[jdduke (slow), 2013-12-02 16:29:42]

lgtm with nits.

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_android.cc
(right):

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:89:
return
Nit: I think this can fit on one line?

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:31: // On
platforms where input events are processed by a gesture recognizer
Nit: remove the sentence with "On platforms", I think it's clear enough with the
virtual method, and this isn't very visible.

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_pinch_gesture.cc (right):

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:88: void
SyntheticPinchGesture::SetupCoordinates(SyntheticGestureTarget* target) {
Nit: Move this below |ForwardTouchEvent()| for consistency with declaration
ordering.

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_pinch_gesture.h (right):

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.h:26:
SyntheticPinchGestureParams params_;
I didn't realize the private vars were before the private methods here.  The
order should be reversed (methods before vars), could you do that in your pinch
gesture refactoring (maybe you already did?)?

[Dominik Grewe, 2013-12-02 16:42:25]

Thanks Jared!

Adding bulach as owner for content/common/android/

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_android.cc
(right):

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:89:
return
On 2013/12/02 16:29:43, jdduke wrote:
> Nit: I think this can fit on one line?

Done.

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:31: // On
platforms where input events are processed by a gesture recognizer
On 2013/12/02 16:29:43, jdduke wrote:
> Nit: remove the sentence with "On platforms", I think it's clear enough with
the
> virtual method, and this isn't very visible.

Done.

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_pinch_gesture.cc (right):

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:88: void
SyntheticPinchGesture::SetupCoordinates(SyntheticGestureTarget* target) {
On 2013/12/02 16:29:43, jdduke wrote:
> Nit: Move this below |ForwardTouchEvent()| for consistency with declaration
> ordering.

Done.

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_pinch_gesture.h (right):

https://codereview.chromium.org/95153002/diff/100001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.h:26:
SyntheticPinchGestureParams params_;
On 2013/12/02 16:29:43, jdduke wrote:
> I didn't realize the private vars were before the private methods here.  The
> order should be reversed (methods before vars), could you do that in your
pinch
> gesture refactoring (maybe you already did?)?

Yeah, I noticed that when refactoring. I'll upload it in a separate patch.

[Dominik Grewe, 2013-12-02 17:17:52]

Just noticed a problem with pinch when total_num_pixels_covered = 0. Because we
add a touch slop, HasFinished() will be false in that case. I've updated the CL
not to use HasFinished but rather check for total_num_pixels_covered directly
(same for smooth scroll although it wasn't an issue there). I've also added unit
tests for that case.

[Dominik Grewe, 2013-12-02 17:36:36]

+bulach

[bulach, 2013-12-02 18:24:50]

lgtm, thanks! just nits below:

https://codereview.chromium.org/95153002/diff/140001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc
(right):

https://codereview.chromium.org/95153002/diff/140001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:75: return
ui::GestureConfiguration::max_touch_move_in_pixels_for_click();
is this "inclusive" ?
as in, is the test < or <=, and should this have + 1 if the latter?

https://codereview.chromium.org/95153002/diff/140001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_pinch_gesture.cc (right):

https://codereview.chromium.org/95153002/diff/140001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:23:
current_y_0_(0),
nit: wouldn't make any difference, but 0.0f would be more indicative these are
floats.

https://codereview.chromium.org/95153002/diff/140001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:97:
target->GetTouchSlopInDips();
nit: "target->GetTouchSlopInDips()" is going to java twice..
perhaps:
const float kTouchSlopInDips = target->GetTouchSlopInDips();
float inner_distance_to_anchor = 2 * kTouchSlopInDips;
float outer_distance_to_anchor = 3 * kTouchSlopInDips +
    params_.total_num_pixels_covered / 2.0f;

[Dominik Grewe, 2013-12-02 18:50:03]

Thanks Marcus!

https://codereview.chromium.org/95153002/diff/140001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc
(right):

https://codereview.chromium.org/95153002/diff/140001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:75: return
ui::GestureConfiguration::max_touch_move_in_pixels_for_click();
On 2013/12/02 18:24:51, bulach wrote:
> is this "inclusive" ?
> as in, is the test < or <=, and should this have + 1 if the latter?

Good point. The test seems to be for < (although the name of the variable
suggests otherwise). But what we actually want is <=, i.e. the distance it can
move without being considered moving; that's what we get for Android (I'll
update the comment in synthetic_gesture_target.h. So it should be - 1 if I'm not
mistaken.

https://codereview.chromium.org/95153002/diff/140001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_pinch_gesture.cc (right):

https://codereview.chromium.org/95153002/diff/140001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:23:
current_y_0_(0),
On 2013/12/02 18:24:51, bulach wrote:
> nit: wouldn't make any difference, but 0.0f would be more indicative these are
> floats.

Done.

https://codereview.chromium.org/95153002/diff/140001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:97:
target->GetTouchSlopInDips();
On 2013/12/02 18:24:51, bulach wrote:
> nit: "target->GetTouchSlopInDips()" is going to java twice..
> perhaps:
> const float kTouchSlopInDips = target->GetTouchSlopInDips();
> float inner_distance_to_anchor = 2 * kTouchSlopInDips;
> float outer_distance_to_anchor = 3 * kTouchSlopInDips +
>     params_.total_num_pixels_covered / 2.0f;

Done.

[commit-bot: I haz the power, 2013-12-02 19:20:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/95153002/170001

[commit-bot: I haz the power, 2013-12-02 19:45:22]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-02 21:24:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/95153002/190001

[commit-bot: I haz the power, 2013-12-03 00:42:10]

Change committed as 238248