Fix TAP_DOWN gesture event:

Fix TAP_DOWN gesture event:
1. GESTURE_TAP_DOWN should be sent only on the first TOUCH_PRESSED.
2. New gesture GESTURE_BEGIN shuold be sent on all TOUCH_PRESSED.
3. Rename GESTURE_TAP_UP to GESTURE_END.

BUG=131007
TEST=manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140967

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141011

[sadrul, 2012-06-06 19:51:13]

https://chromiumcodereview.appspot.com/10533029/diff/1/ash/wm/system_gesture_...
File ash/wm/system_gesture_event_filter.cc (right):

https://chromiumcodereview.appspot.com/10533029/diff/1/ash/wm/system_gesture_...
ash/wm/system_gesture_event_filter.cc:538: if (event->type() ==
ui::ET_GESTURE_TAP_DOWN &&
Change this to _BEGIN

https://chromiumcodereview.appspot.com/10533029/diff/1/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://chromiumcodereview.appspot.com/10533029/diff/1/content/browser/render...
content/browser/renderer_host/render_widget_host_view_win.cc:380: case
ui::ET_GESTURE_END:
Same for _BEGIN

https://chromiumcodereview.appspot.com/10533029/diff/1/content/browser/render...
File content/browser/renderer_host/web_input_event_aurax11.cc (right):

https://chromiumcodereview.appspot.com/10533029/diff/1/content/browser/render...
content/browser/renderer_host/web_input_event_aurax11.cc:371: case
ui::ET_GESTURE_END:
_BEGIN too

https://chromiumcodereview.appspot.com/10533029/diff/1/ui/base/gestures/gestu...
File ui/base/gestures/gesture_sequence.cc (right):

https://chromiumcodereview.appspot.com/10533029/diff/1/ui/base/gestures/gestu...
ui/base/gestures/gesture_sequence.cc:483: point_count_, 0.f, 1 <<
point.touch_id()));
We don't need to send point_count_ here anymore. So let's remove it to make it 0
(just to make sure nothing is using/depending on this value being 1)

https://chromiumcodereview.appspot.com/10533029/diff/1/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://chromiumcodereview.appspot.com/10533029/diff/1/ui/views/widget/widget...
ui/views/widget/widget.cc:1120: is_touch_down_ = false;
Oh. This is wrong. This should check for delta_x() == 1 to make sure releasing
the second/third finger doesn't reset this.

[varunjain, 2012-06-06 21:52:53]

https://chromiumcodereview.appspot.com/10533029/diff/1/ash/wm/system_gesture_...
File ash/wm/system_gesture_event_filter.cc (right):

https://chromiumcodereview.appspot.com/10533029/diff/1/ash/wm/system_gesture_...
ash/wm/system_gesture_event_filter.cc:538: if (event->type() ==
ui::ET_GESTURE_TAP_DOWN &&
On 2012/06/06 19:51:14, sadrul wrote:
> Change this to _BEGIN

Done.

https://chromiumcodereview.appspot.com/10533029/diff/1/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://chromiumcodereview.appspot.com/10533029/diff/1/content/browser/render...
content/browser/renderer_host/render_widget_host_view_win.cc:380: case
ui::ET_GESTURE_END:
On 2012/06/06 19:51:14, sadrul wrote:
> Same for _BEGIN

Done.

https://chromiumcodereview.appspot.com/10533029/diff/1/content/browser/render...
File content/browser/renderer_host/web_input_event_aurax11.cc (right):

https://chromiumcodereview.appspot.com/10533029/diff/1/content/browser/render...
content/browser/renderer_host/web_input_event_aurax11.cc:371: case
ui::ET_GESTURE_END:
On 2012/06/06 19:51:14, sadrul wrote:
> _BEGIN too

Done.

https://chromiumcodereview.appspot.com/10533029/diff/1/ui/base/gestures/gestu...
File ui/base/gestures/gesture_sequence.cc (right):

https://chromiumcodereview.appspot.com/10533029/diff/1/ui/base/gestures/gestu...
ui/base/gestures/gesture_sequence.cc:483: point_count_, 0.f, 1 <<
point.touch_id()));
On 2012/06/06 19:51:14, sadrul wrote:
> We don't need to send point_count_ here anymore. So let's remove it to make it
0
> (just to make sure nothing is using/depending on this value being 1)

Done.

https://chromiumcodereview.appspot.com/10533029/diff/1/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://chromiumcodereview.appspot.com/10533029/diff/1/ui/views/widget/widget...
ui/views/widget/widget.cc:1120: is_touch_down_ = false;
On 2012/06/06 19:51:14, sadrul wrote:
> Oh. This is wrong. This should check for delta_x() == 1 to make sure releasing
> the second/third finger doesn't reset this.

Done.

[sadrul, 2012-06-06 22:13:40]

LGTM. Thanks!

[sky, 2012-06-07 02:43:54]

http://codereview.chromium.org/10533029/diff/8001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

http://codereview.chromium.org/10533029/diff/8001/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:1120: if (event.delta_x() == 1) {
I don't understand the delta_x check? Why wouldn't we do this all the time?

[sadrul, 2012-06-07 03:04:54]

http://codereview.chromium.org/10533029/diff/8001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

http://codereview.chromium.org/10533029/diff/8001/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:1120: if (event.delta_x() == 1) {
On 2012/06/07 02:43:54, sky wrote:
> I don't understand the delta_x check? Why wouldn't we do this all the time?

delta_x() means the number of active points for GESTURE_END. So if you have
three fingers down, releasing one of the fingers will generate a GESTURE_END
with delta_x() == 3. So GESTURE_END with delta_x() == 1 would imply that there
is currently no touch-down (and delta_x() > 1 implies at least some touch-point
is active)

(Naming this delta_x/delta_y is a bit unfortunate, and done for historical
reasons (to match with WebKit). These are planned to be fixed, eventually).

[sky, 2012-06-07 03:31:56]

Oy, I hope that name gets fixed.

I had considered doing something like this, but we don't do the same
for mouse events. But, I suppose this shouldn't cause problems so LGTM

On Wed, Jun 6, 2012 at 8:04 PM,  <sadrul@chromium.org> wrote:
>
> http://codereview.chromium.org/10533029/diff/8001/ui/views/widget/widget.cc
> File ui/views/widget/widget.cc (right):
>
>
http://codereview.chromium.org/10533029/diff/8001/ui/views/widget/widget.cc#n...
> ui/views/widget/widget.cc:1120: if (event.delta_x() == 1) {
> On 2012/06/07 02:43:54, sky wrote:
>>
>> I don't understand the delta_x check? Why wouldn't we do this all the
>
> time?
>
> delta_x() means the number of active points for GESTURE_END. So if you
> have three fingers down, releasing one of the fingers will generate a
> GESTURE_END with delta_x() == 3. So GESTURE_END with delta_x() == 1
> would imply that there is currently no touch-down (and delta_x() > 1
> implies at least some touch-point is active)
>
> (Naming this delta_x/delta_y is a bit unfortunate, and done for
> historical reasons (to match with WebKit). These are planned to be
> fixed, eventually).
>
> http://codereview.chromium.org/10533029/

[commit-bot: I haz the power, 2012-06-07 03:46:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10533029/8001

[commit-bot: I haz the power, 2012-06-07 05:56:17]

Change committed as 140967

[commit-bot: I haz the power, 2012-06-07 13:31:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10533029/4005

[commit-bot: I haz the power, 2012-06-07 15:33:07]

Change committed as 141011