Avoid an extra GestureDetector instance for double-tap drag zooming

Avoid an extra GestureDetector instance for double-tap drag zooming

Currently, the ScaleGestureDetector will create a standalone GestureDetector
instance for detecting double-tap events. This is overkill, however,
as the only real callback it requires is the one for the initial |OnDoubleTap|,
and running detection for all other gestures is wasteful. Avoid this by
consolidating the GestureListener and ScaleGestureListener constructs, instead
exposing an explicit |ScaleGestureDetector::OnDoubleTap()| method that is called
indirectly using the existing GestureDetector instance.

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

[jdduke (slow), 2014-08-22 17:48:07]

tdresser@: This looks big, but it's really just moving around some bits. Now we
avoid calling back into the provider from the listener entirely, and I think the
flow is a bit easier to follow (not to mention a nice reduction in LOC). There
should be no meaningful change in observable behavior, so I'm hoping with
existing test coverage we'll be (relatively) safe with this transformation. Let
me know if you'd prefer somebody else do the reviewing. Thanks.

[tdresser, 2014-08-22 18:19:05]

LGTM, with a few comments on the comments.

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

https://codereview.chromium.org/501503003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:389: if
(snap_scroll_controller_.IsSnapHorizontal()) {
While you're here, you could switch to a braceless if.

https://codereview.chromium.org/501503003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:566: // gesture detector not to
emit further scaling notifications
Might as well reword this comment while you're here.

At least:
"not to emit" -> "not emit"
"if detector events" -> "if multitouch zoom events"

On second thought, does this comment even make sense at this point? There's
nothing to worry about keeping in sync as of this patch... You can probably just
get rid of this comment.

https://codereview.chromium.org/501503003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:619: // Tracks whether
{PINCH|SCROLL|_BEGIN events have been forwadred for the
forwadred -> forwarded
{PINCH|SCROLL|_BEGIN -> ...

https://codereview.chromium.org/501503003/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/scale_gesture_detector.h (right):

https://codereview.chromium.org/501503003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/scale_gesture_detector.h:25: // Distance a span can
deviate before scaling will start (in dips).
Reword this comment. (At least define span)

[jdduke (slow), 2014-08-22 20:33:10]

Thanks for review!

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

https://codereview.chromium.org/501503003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:389: if
(snap_scroll_controller_.IsSnapHorizontal()) {
On 2014/08/22 18:19:04, tdresser wrote:
> While you're here, you could switch to a braceless if.

Done.

https://codereview.chromium.org/501503003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:566: // gesture detector not to
emit further scaling notifications
On 2014/08/22 18:19:04, tdresser wrote:
> Might as well reword this comment while you're here.
> 
> At least:
> "not to emit" -> "not emit"
> "if detector events" -> "if multitouch zoom events"
> 
> On second thought, does this comment even make sense at this point? There's
> nothing to worry about keeping in sync as of this patch... You can probably
just
> get rid of this comment.

Ah, yeah this can be cleaned up. I think we still want this behavior, but the
wording here is pretty bad.

https://codereview.chromium.org/501503003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/gesture_provider.cc:619: // Tracks whether
{PINCH|SCROLL|_BEGIN events have been forwadred for the
On 2014/08/22 18:19:04, tdresser wrote:
> forwadred -> forwarded
> {PINCH|SCROLL|_BEGIN -> ...

Done.

https://codereview.chromium.org/501503003/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/scale_gesture_detector.h (right):

https://codereview.chromium.org/501503003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/scale_gesture_detector.h:25: // Distance a span can
deviate before scaling will start (in dips).
On 2014/08/22 18:19:04, tdresser wrote:
> Reword this comment. (At least define span)

Done.

https://codereview.chromium.org/501503003/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/scale_gesture_detector.h:25: // Distance a span can
deviate before scaling will start (in dips).
On 2014/08/22 18:19:04, tdresser wrote:
> Reword this comment. (At least define span)

Done.

[jdduke (slow), 2014-08-22 20:33:15]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-08-22 20:34:37]

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

[tdresser, 2014-08-22 20:45:40]

Spelling nit. Feel free to land however you see fit.

https://codereview.chromium.org/501503003/diff/60001/ui/events/gesture_detect...
File ui/events/gesture_detection/scale_gesture_detector.h (right):

https://codereview.chromium.org/501503003/diff/60001/ui/events/gesture_detect...
ui/events/gesture_detection/scale_gesture_detector.h:27: // a radius of average
pointer deviation from the focial point.
focial -> focal

[jdduke (slow), 2014-08-22 21:26:11]

The CQ bit was unchecked by jdduke@chromium.org

[jdduke (slow), 2014-08-22 21:26:50]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2014-08-22 21:26:55]

https://codereview.chromium.org/501503003/diff/60001/ui/events/gesture_detect...
File ui/events/gesture_detection/scale_gesture_detector.h (right):

https://codereview.chromium.org/501503003/diff/60001/ui/events/gesture_detect...
ui/events/gesture_detection/scale_gesture_detector.h:27: // a radius of average
pointer deviation from the focial point.
On 2014/08/22 20:45:40, tdresser wrote:
> focial -> focal

Bah, yeah I'll do it now.

[commit-bot: I haz the power, 2014-08-22 21:32:28]

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

[commit-bot: I haz the power, 2014-08-22 22:30:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-22 23:33:38]

Committed patchset #4 (80001) as 291555