Implementing the dispatch of the swipe gesture for one-finger touch swipes.

Implementing the dispatch of the swipe gesture for one-finger touch swipes.

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

[mfomitchev, 2014-05-26 20:06:09]

Can you please review and if you are okay with this I will add the rest of the
owners. Thanks!

[tdresser, 2014-05-26 20:49:57]

LGTM with nits.

https://codereview.chromium.org/302463004/diff/1/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/302463004/diff/1/ash/touch/touch_uma.cc#newco...
ash/touch/touch_uma.cc:173: return UMA_ET_GESTURE_MULTIFINGER_SWIPE_1;
Can we switch to testing in ascending order instead of descending? That way the
most general condition (>= 4) is handled by the else block.

https://codereview.chromium.org/302463004/diff/1/ash/wm/toplevel_window_event...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/302463004/diff/1/ash/wm/toplevel_window_event...
ash/wm/toplevel_window_event_handler.cc:348: if (event->details().touch_points()
<= 1)
I'd split out the invalid case here.

DCHECK(event->details().touch_points() > 0)
if (event->details() == 1)
  return;

https://codereview.chromium.org/302463004/diff/1/ui/events/event_constants.h
File ui/events/event_constants.h (right):

https://codereview.chromium.org/302463004/diff/1/ui/events/event_constants.h#...
ui/events/event_constants.h:49: // A SWIPE gesture can happen at the end of a
TAP_UP gesture if the
This comment still kinda implies that multiple fingers are required. (And
there's no such thing as a TAP_UP gesture).

Perhaps:

// A SWIPE gesture can happen at the end of a touch sequence if 
// the finger velocity was high enough when the first finger was  
// released.

[jdduke (slow), 2014-05-26 21:19:42]

lgtm with nits.  I suppose it's fine that the fling event precedes the swipe
event?

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

https://codereview.chromium.org/302463004/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_detector.cc:554: if (ratio >
min_swipe_direction_component_ratio_) {
Nit: I would early out here in the unsatisfied case, i.e.,

if (ratio < min_swipe_direction_component_ratio_)
  return false;

...

(Note that the inequality strictness doesn't really matter, either way should
work).

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

https://codereview.chromium.org/302463004/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_detector.h:160: bool
handleSwipeIfNeeded(const MotionEvent& up, float vx, float vy);
Nit: handle -> Handle

[mfomitchev, 2014-05-26 22:07:14]

https://codereview.chromium.org/302463004/diff/1/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/302463004/diff/1/ash/touch/touch_uma.cc#newco...
ash/touch/touch_uma.cc:173: return UMA_ET_GESTURE_MULTIFINGER_SWIPE_1;
Ok. I am going to do the same for UMA_ET_GESTURE_SCROLL_UPDATE then to be
consistent.

https://codereview.chromium.org/302463004/diff/1/ash/wm/toplevel_window_event...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/302463004/diff/1/ash/wm/toplevel_window_event...
ash/wm/toplevel_window_event_handler.cc:348: if (event->details().touch_points()
<= 1)
On 2014/05/26 20:49:58, tdresser wrote:
> I'd split out the invalid case here.
> 
> DCHECK(event->details().touch_points() > 0)
> if (event->details() == 1)
>   return;

Done.

https://codereview.chromium.org/302463004/diff/1/ui/events/event_constants.h
File ui/events/event_constants.h (right):

https://codereview.chromium.org/302463004/diff/1/ui/events/event_constants.h#...
ui/events/event_constants.h:49: // A SWIPE gesture can happen at the end of a
TAP_UP gesture if the
On 2014/05/26 20:49:58, tdresser wrote:
> This comment still kinda implies that multiple fingers are required. (And
> there's no such thing as a TAP_UP gesture).
> 
> Perhaps:
> 
> // A SWIPE gesture can happen at the end of a touch sequence if 
> // the finger velocity was high enough when the first finger was  
> // released.

Done.

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

https://codereview.chromium.org/302463004/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_detector.cc:554: if (ratio >
min_swipe_direction_component_ratio_) {
On 2014/05/26 21:19:42, jdduke wrote:
> Nit: I would early out here in the unsatisfied case, i.e.,
> 
> if (ratio < min_swipe_direction_component_ratio_)
>   return false;
> 
> ...
> 
> (Note that the inequality strictness doesn't really matter, either way should
> work).

Done.

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

https://codereview.chromium.org/302463004/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_detector.h:160: bool
handleSwipeIfNeeded(const MotionEvent& up, float vx, float vy);
On 2014/05/26 21:19:42, jdduke wrote:
> Nit: handle -> Handle

Done.

[mfomitchev, 2014-05-26 22:10:54]

mpearson@chromium.org: Please review changes in 
histograms.xml

sky@chromium.org: Please review changes in 
remaining files

[tdresser, 2014-05-27 12:07:31]

https://codereview.chromium.org/302463004/diff/20001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/302463004/diff/20001/ash/touch/touch_uma.cc#n...
ash/touch/touch_uma.cc:123: return UMA_ET_GESTURE_SCROLL_UPDATE_4P;
Thanks for catching this case - I think this does read a somewhat nicer.

[sky, 2014-05-27 17:27:54]

LGTM

https://codereview.chromium.org/302463004/diff/20001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/302463004/diff/20001/ash/touch/touch_uma.cc#n...
ash/touch/touch_uma.cc:47: UMA_ET_GESTURE_MULTIFINGER_SWIPE_2,
Document what this and swipe 1 is.

https://codereview.chromium.org/302463004/diff/20001/ash/touch/touch_uma.cc#n...
ash/touch/touch_uma.cc:47: UMA_ET_GESTURE_MULTIFINGER_SWIPE_2,
Can you move all the UMA_ET_GESTURE_MULTIFINGER_SWIPE together?

https://codereview.chromium.org/302463004/diff/20001/ash/wm/toplevel_window_e...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/302463004/diff/20001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler.cc:348:
DCHECK(event->details().touch_points() > 0);
DCHECK_GT

[Alexei Svitkine (slow), 2014-05-27 17:29:11]

https://codereview.chromium.org/302463004/diff/20001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/302463004/diff/20001/ash/touch/touch_uma.cc#n...
ash/touch/touch_uma.cc:47: UMA_ET_GESTURE_MULTIFINGER_SWIPE_2,
On 2014/05/27 17:27:54, sky wrote:
> Can you move all the UMA_ET_GESTURE_MULTIFINGER_SWIPE together?

If this is used in UMA enum histograms (which it looks like), it's incorrect to
renumber them.

[sky, 2014-05-27 17:36:33]

https://codereview.chromium.org/302463004/diff/20001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/302463004/diff/20001/ash/touch/touch_uma.cc#n...
ash/touch/touch_uma.cc:47: UMA_ET_GESTURE_MULTIFINGER_SWIPE_2,
On 2014/05/27 17:29:12, Alexei Svitkine wrote:
> On 2014/05/27 17:27:54, sky wrote:
> > Can you move all the UMA_ET_GESTURE_MULTIFINGER_SWIPE together?
> 
> If this is used in UMA enum histograms (which it looks like), it's incorrect
to
> renumber them.

Bummer. If that is the case we should have that warning at the top and I think
explicit values make it even clearer.

[Mark P, 2014-05-27 18:16:01]

https://codereview.chromium.org/302463004/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/302463004/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:43238: +  <int value="35"
label="One-finger swipe"/>
It doesn't make sense to me that the touch_uma.cc event type for this is
UMA_ET_GESTURE_MULTIFINGER_SWIPE_1.  That doesn't seem to be in agreement with
this label.

[mfomitchev, 2014-05-27 18:33:43]

https://codereview.chromium.org/302463004/diff/20001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/302463004/diff/20001/ash/touch/touch_uma.cc#n...
ash/touch/touch_uma.cc:47: UMA_ET_GESTURE_MULTIFINGER_SWIPE_2,
On 2014/05/27 17:27:54, sky wrote:
> Document what this and swipe 1 is.

Done.

https://codereview.chromium.org/302463004/diff/20001/ash/touch/touch_uma.cc#n...
ash/touch/touch_uma.cc:47: UMA_ET_GESTURE_MULTIFINGER_SWIPE_2,
On 2014/05/27 17:36:33, sky wrote:
> On 2014/05/27 17:29:12, Alexei Svitkine wrote:
> > On 2014/05/27 17:27:54, sky wrote:
> > > Can you move all the UMA_ET_GESTURE_MULTIFINGER_SWIPE together?
> > 
> > If this is used in UMA enum histograms (which it looks like), it's incorrect
> to
> > renumber them.
> 
> Bummer. If that is the case we should have that warning at the top and I think
> explicit values make it even clearer.

Done.

https://codereview.chromium.org/302463004/diff/20001/ash/wm/toplevel_window_e...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/302463004/diff/20001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler.cc:348:
DCHECK(event->details().touch_points() > 0);
On 2014/05/27 17:27:54, sky wrote:
> DCHECK_GT

Done.

https://codereview.chromium.org/302463004/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/302463004/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:43238: +  <int value="35"
label="One-finger swipe"/>
Ok, I have removed "MULTIFINGER" from the type names

[Mark P, 2014-05-27 19:25:16]

histograms.xml lgtm

[mfomitchev, 2014-05-27 21:21:58]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-05-27 21:22:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/302463004/40001

[commit-bot: I haz the power, 2014-05-28 01:13:38]

Change committed as 273097