Provide max gesture bounds and option to ignore touch size during pinch

Provide max gesture bounds and option to ignore touch size during pinch

Certain devices produce garbage |MotionEvent.getTouchMajor()| values, either
unreasonably small or unreasonable large, with extreme variance between touches.
This can cause a number of problems with the current gesture detection pipeline.

Add a maximum gesture bounds size as a sanity fallback for tap disambiguation.
Also disable by default the use of touch size when calculating the pinch scale
factor. Its use does not seem to add any measurable benefit, but can produce
severe scale artifacts on devices with unpredictable touch sizes. This
phenomenon appears to be more common on devices originally targetted for ICS.

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

[jdduke (slow), 2014-06-29 01:53:10]

tdresser@: PTAL.  I haven't really finalized this yet, but I wanted to get your
thoughts.

A part of me wants to rip out the touch major usage altogether in
ScaleGestureDetector.  It seems to be a fountain of bugs, even judging from the
original Android ScaleGestureDetector.java revision history.  I'm somewhat
hesitant to do so immediately, perhaps we can remove it entirely after these
changes have baked for a while?

[tdresser, 2014-07-02 13:43:10]

Seems like a reasonable approach.

I'm in favor of getting rid of all touch dimension information from the
ScaleGestureDetector.

Have you determined why it was added initially?

https://codereview.chromium.org/340343013/diff/80001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/gfx/ViewConfigurationHelper.java
(right):

https://codereview.chromium.org/340343013/diff/80001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gfx/ViewConfigurationHelper.java:139:
private static boolean getTouchMajorUsedInScalingSpan() {
This method name makes it sound like you're returning the touch major, not
whether or not the touch major should be used.

Perhaps |isTouchMajorUsedInScalingSpan|?

Or |getUsingTouchMajorInScalingSpan|.

https://codereview.chromium.org/340343013/diff/80001/ui/events/gesture_detect...
File ui/events/gesture_detection/gesture_provider_unittest.cc (right):

https://codereview.chromium.org/340343013/diff/80001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider_unittest.cc:1572: // Verify that
pinch zoom that doesn't use touch major values sends the proper
It doesn't look like this is testing the case where we don't use touch major
values...

https://codereview.chromium.org/340343013/diff/80001/ui/events/gesture_detect...
File ui/events/gesture_detection/scale_gesture_detector.cc (right):

https://codereview.chromium.org/340343013/diff/80001/ui/events/gesture_detect...
ui/events/gesture_detection/scale_gesture_detector.cc:92: std::max(kSlopEpsilon,
config.min_scaling_span - touch_max_major_);
I'm not clear on why you're subtracting |touch_max_major_| here.
Could you add a comment?

[tdresser, 2014-07-02 13:46:29]

On 2014/07/02 13:43:10, tdresser wrote:
> Seems like a reasonable approach.
> 
> I'm in favor of getting rid of all touch dimension information from the
> ScaleGestureDetector.
> 
> Have you determined why it was added initially?
> 
>
https://codereview.chromium.org/340343013/diff/80001/ui/android/java/src/org/...
> File ui/android/java/src/org/chromium/ui/gfx/ViewConfigurationHelper.java
> (right):
> 
>
https://codereview.chromium.org/340343013/diff/80001/ui/android/java/src/org/...
> ui/android/java/src/org/chromium/ui/gfx/ViewConfigurationHelper.java:139:
> private static boolean getTouchMajorUsedInScalingSpan() {
> This method name makes it sound like you're returning the touch major, not
> whether or not the touch major should be used.
> 
> Perhaps |isTouchMajorUsedInScalingSpan|?
> 
> Or |getUsingTouchMajorInScalingSpan|.
> 
>
https://codereview.chromium.org/340343013/diff/80001/ui/events/gesture_detect...
> File ui/events/gesture_detection/gesture_provider_unittest.cc (right):
> 
>
https://codereview.chromium.org/340343013/diff/80001/ui/events/gesture_detect...
> ui/events/gesture_detection/gesture_provider_unittest.cc:1572: // Verify that
> pinch zoom that doesn't use touch major values sends the proper
> It doesn't look like this is testing the case where we don't use touch major
> values...
> 
>
https://codereview.chromium.org/340343013/diff/80001/ui/events/gesture_detect...
> File ui/events/gesture_detection/scale_gesture_detector.cc (right):
> 
>
https://codereview.chromium.org/340343013/diff/80001/ui/events/gesture_detect...
> ui/events/gesture_detection/scale_gesture_detector.cc:92:
std::max(kSlopEpsilon,
> config.min_scaling_span - touch_max_major_);
> I'm not clear on why you're subtracting |touch_max_major_| here.
> Could you add a comment?

Ignore my question about why it was added initially, just saw the email thread.

[jdduke (slow), 2014-07-02 16:24:18]

https://codereview.chromium.org/340343013/diff/80001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/gfx/ViewConfigurationHelper.java
(right):

https://codereview.chromium.org/340343013/diff/80001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/gfx/ViewConfigurationHelper.java:139:
private static boolean getTouchMajorUsedInScalingSpan() {
On 2014/07/02 13:43:10, tdresser wrote:
> This method name makes it sound like you're returning the touch major, not
> whether or not the touch major should be used.
> 
> Perhaps |isTouchMajorUsedInScalingSpan|?
> 
> Or |getUsingTouchMajorInScalingSpan|.

Good point, renamed to |ShouldUseTouchMajorInScalingSpan|.

https://codereview.chromium.org/340343013/diff/80001/ui/events/gesture_detect...
File ui/events/gesture_detection/gesture_provider_unittest.cc (right):

https://codereview.chromium.org/340343013/diff/80001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider_unittest.cc:1572: // Verify that
pinch zoom that doesn't use touch major values sends the proper
On 2014/07/02 13:43:10, tdresser wrote:
> It doesn't look like this is testing the case where we don't use touch major
> values...

Oops, the comment is wrong, good catch.

https://codereview.chromium.org/340343013/diff/80001/ui/events/gesture_detect...
File ui/events/gesture_detection/scale_gesture_detector.cc (right):

https://codereview.chromium.org/340343013/diff/80001/ui/events/gesture_detect...
ui/events/gesture_detection/scale_gesture_detector.cc:92: std::max(kSlopEpsilon,
config.min_scaling_span - touch_max_major_);
On 2014/07/02 13:43:10, tdresser wrote:
> I'm not clear on why you're subtracting |touch_max_major_| here.
> Could you add a comment?

Certainly, and I've moved the subtraction logic out and into
GestureConfigHelperAndroid.

[tdresser, 2014-07-02 16:28:49]

LGTM.

[jdduke (slow), 2014-07-02 16:51:40]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-07-02 16:52:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/340343013/100001

[commit-bot: I haz the power, 2014-07-02 19:36:05]

Change committed as 281078