[Aura] Reduce frequency of PinchUpdate events.

[Aura] Reduce frequency of PinchUpdate events.

BUG=373318
TEST=GestureProviderTest.PinchZoomWithThreshold

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

[jdduke (slow), 2014-05-21 19:30:37]

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/gesture_provider_unittest.cc (right):

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider_unittest.cc:1952:
event.SetId(++motion_event_id);
Nit: Let's remove all of the touch id assignment and verification in this test,
it's just noise.

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider_unittest.cc:1993: const float
kOvershootMinPinchUpdateDistance = 3;
Hmm, where do you get 3 from?

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/scale_gesture_detector.cc (right):

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/scale_gesture_detector.cc:211: std::abs(span -
prev_span_) >= min_pinch_update_distance_;
Can we move this check into the ACTION_MOVE portion below?

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/scale_gesture_detector.h (right):

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/scale_gesture_detector.h:39: float
min_pinch_update_distance;
Maybe call this |min_pinch_update_span_delta|?

[tdresser, 2014-05-22 12:31:44]

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/gesture_provider_unittest.cc (right):

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider_unittest.cc:1952:
event.SetId(++motion_event_id);
On 2014/05/21 19:30:37, jdduke wrote:
> Nit: Let's remove all of the touch id assignment and verification in this
test,
> it's just noise.

Done.

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_provider_unittest.cc:1993: const float
kOvershootMinPinchUpdateDistance = 3;
On 2014/05/21 19:30:37, jdduke wrote:
> Hmm, where do you get 3 from?

It comes from the way that touch radius factors into the span calculation. (I
made it up). Do you think I should figure out how to derive the correct constant
based on the way span is calculated?

I think just using something generous, but less than kMinPinchUpdateDistance is
probably fine.

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/scale_gesture_detector.cc (right):

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/scale_gesture_detector.cc:211: std::abs(span -
prev_span_) >= min_pinch_update_distance_;
On 2014/05/21 19:30:37, jdduke wrote:
> Can we move this check into the ACTION_MOVE portion below?

We need to read prev_span_ before it is modified below.

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/scale_gesture_detector.cc:218: prev_span_ =
curr_span_ = span;
Need to read prev_span_ before this line.

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/scale_gesture_detector.h (right):

https://codereview.chromium.org/298823006/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/scale_gesture_detector.h:39: float
min_pinch_update_distance;
On 2014/05/21 19:30:37, jdduke wrote:
> Maybe call this |min_pinch_update_span_delta|?

Done.

[jdduke (slow), 2014-05-22 17:37:23]

https://codereview.chromium.org/298823006/diff/40001/ui/events/gesture_detect...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/298823006/diff/40001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:178: bool pinch_occured =
Hmm, why are you doing this before the if (!pinch_event_sent_) check? I thought
that's the whole reason we moved this logic here, so that we could get the
PINCH_BEGIN without the PINCH_UPDATE?

I would move this down, before the scale check. Then, just return false if it
fails the threshold test, and use the |GetPreviousSpan()| value from the
ScaleGestureDetector.

https://codereview.chromium.org/298823006/diff/40001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:266: float prev_span_;
See comment above, we shouldn't need to cache the prev_span_.

[tdresser, 2014-05-22 17:57:22]

https://codereview.chromium.org/298823006/diff/40001/ui/events/gesture_detect...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/298823006/diff/40001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:178: bool pinch_occured =
On 2014/05/22 17:37:24, jdduke wrote:
> Hmm, why are you doing this before the if (!pinch_event_sent_) check? I
thought
> that's the whole reason we moved this logic here, so that we could get the
> PINCH_BEGIN without the PINCH_UPDATE?
> 
> I would move this down, before the scale check. Then, just return false if it
> fails the threshold test, and use the |GetPreviousSpan()| value from the
> ScaleGestureDetector.

Done.

https://codereview.chromium.org/298823006/diff/40001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:266: float prev_span_;
On 2014/05/22 17:37:24, jdduke wrote:
> See comment above, we shouldn't need to cache the prev_span_.

Done.

[jdduke (slow), 2014-05-22 18:06:35]

lgtm

https://codereview.chromium.org/298823006/diff/60001/ui/events/gesture_detect...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/298823006/diff/60001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:119:
gesture_begin_end_types_enabled(false) {}
Let's initialize the min_pinch_update_span_delta to 0.

https://codereview.chromium.org/298823006/diff/60001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:190: 
You might consider merging all of this into:

if (std::abs(detector.GetCurrentSpan() - detector.GetPreviousSpan()) <
min_pinch_update_span_delta_)) {
  return false;
}

[tdresser, 2014-05-22 18:25:17]

https://codereview.chromium.org/298823006/diff/60001/ui/events/gesture_detect...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/298823006/diff/60001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:119:
gesture_begin_end_types_enabled(false) {}
On 2014/05/22 18:06:35, jdduke wrote:
> Let's initialize the min_pinch_update_span_delta to 0.

It's in ScaleGestureDetector::Config, but done.

https://codereview.chromium.org/298823006/diff/60001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:190: 
On 2014/05/22 18:06:35, jdduke wrote:
> You might consider merging all of this into:
> 
> if (std::abs(detector.GetCurrentSpan() - detector.GetPreviousSpan()) <
> min_pinch_update_span_delta_)) {
>   return false;
> }

Done.

[tdresser, 2014-05-22 18:25:28]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-05-22 18:29:52]

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

[commit-bot: I haz the power, 2014-05-22 21:46:01]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-05-22 22:04:08]

Change committed as 272336