Bug 67645 - Split Tap gesture detection into TapDown and TapRelease
Summary: Split Tap gesture detection into TapDown and TapRelease
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Varun Jain
URL:
Keywords:
Depends on:
Blocks: 67709
  Show dependency treegraph
 
Reported: 2011-09-06 07:34 PDT by Varun Jain
Modified: 2011-09-08 11:29 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.50 KB, patch)
2011-09-06 07:34 PDT, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (5.27 KB, patch)
2011-09-06 08:27 PDT, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (9.11 KB, patch)
2011-09-06 16:20 PDT, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (10.48 KB, patch)
2011-09-07 12:15 PDT, Varun Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Varun Jain 2011-09-06 07:34:19 PDT
Split Tap gesture detection into TapDown and TapRelease
Comment 1 Varun Jain 2011-09-06 07:34:52 PDT
Created attachment 106416 [details]
Patch
Comment 2 Robert Kroeger 2011-09-06 07:55:16 PDT
Comment on attachment 106416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106416&action=review

> Source/WebCore/page/EventHandler.cpp:2239
> +    case PlatformGestureEvent::TapDownType:

you should have removed click

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:100
> +void GestureRecognizerChromium::appendTapReleaseGestureEvent(const PlatformTouchPoint& touchPoint, Gestures gestures)

why have you not deleted appendClick*?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:182
> +    appendTapReleaseGestureEvent(point, gestures);

I do not believe that TapRelease should be generated here. ScrollEnd already signifies that the finger has left the screen surface and has left it from a scroll gesture. This additional event is wrong.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:71
>      void appendClickGestureEvent(const PlatformTouchPoint&, Gestures);

this one... why is it still here?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:93
> +    IntPoint m_lastTouchScreenPosition;

this variable is superfluous as you have the desired value from the PlatformTouchPoint event in appendTapRelease* Why?

> Source/WebCore/platform/chromium/PopupContainer.cpp:316
> +    case PlatformGestureEvent::TapDownType:

you should have removed click type
Comment 3 WebKit Review Bot 2011-09-06 07:56:15 PDT
Comment on attachment 106416 [details]
Patch

Attachment 106416 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9592855
Comment 4 Varun Jain 2011-09-06 08:16:01 PDT
(In reply to comment #2)
> (From update of attachment 106416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106416&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:2239
> > +    case PlatformGestureEvent::TapDownType:
> 
> you should have removed click

I think TapType is required for EventHandler to differentiate between TapRelease and click. I agree that TapRelease is redundant right now. However, what if a webapp wants to know about TapRelease (for instance in a drag and drop session)? ScrollBegin/End probably encapsulates that case so I am not sure about it. I will remove TapRelease for now and we can add it later if required.

> 
> > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:100
> > +void GestureRecognizerChromium::appendTapReleaseGestureEvent(const PlatformTouchPoint& touchPoint, Gestures gestures)
> 
> why have you not deleted appendClick*?
> 
> > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:182
> > +    appendTapReleaseGestureEvent(point, gestures);
> 
> I do not believe that TapRelease should be generated here. ScrollEnd already signifies that the finger has left the screen surface and has left it from a scroll gesture. This additional event is wrong.
> 
> > Source/WebCore/platform/chromium/GestureRecognizerChromium.h:71
> >      void appendClickGestureEvent(const PlatformTouchPoint&, Gestures);
> 
> this one... why is it still here?
> 
> > Source/WebCore/platform/chromium/GestureRecognizerChromium.h:93
> > +    IntPoint m_lastTouchScreenPosition;
> 
> this variable is superfluous as you have the desired value from the PlatformTouchPoint event in appendTapRelease* Why?
> 
> > Source/WebCore/platform/chromium/PopupContainer.cpp:316
> > +    case PlatformGestureEvent::TapDownType:
> 
> you should have removed click type
Comment 5 Varun Jain 2011-09-06 08:27:33 PDT
Created attachment 106420 [details]
Patch
Comment 6 Robert Kroeger 2011-09-06 12:18:04 PDT
Comment on attachment 106420 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106420&action=review

the fact that the unit tests haven't been updated would seem to indicate that I didn't write enough unit tests. Perhaps you could add some?

Also: the why of this is important. You'll want to align this with qt port's work on adding the link preview.

> Source/WebCore/page/EventHandler.cpp:2240
> +    case PlatformGestureEvent::TapDownType:

nit: TapDown should come above TapType.

> Source/WebCore/platform/PlatformGestureEvent.h:42
> +        TapType,

this re-ordering would break a binary interface. Please leave TapType unchanged.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:70
>      void appendClickGestureEvent(const PlatformTouchPoint&, Gestures);

might be nice to rename this to appendTapGestureEvent? Am not sure it matters.
Comment 7 Varun Jain 2011-09-06 16:20:03 PDT
(In reply to comment #6)
> (From update of attachment 106420 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106420&action=review
> 
> the fact that the unit tests haven't been updated would seem to indicate that I didn't write enough unit tests. Perhaps you could add some?

Done.

> 
> Also: the why of this is important. You'll want to align this with qt port's work on adding the link preview.
> 
> > Source/WebCore/page/EventHandler.cpp:2240
> > +    case PlatformGestureEvent::TapDownType:
> 
> nit: TapDown should come above TapType.

Done.

> 
> > Source/WebCore/platform/PlatformGestureEvent.h:42
> > +        TapType,
> 
> this re-ordering would break a binary interface. Please leave TapType unchanged.

Done.

> 
> > Source/WebCore/platform/chromium/GestureRecognizerChromium.h:70
> >      void appendClickGestureEvent(const PlatformTouchPoint&, Gestures);
> 
> might be nice to rename this to appendTapGestureEvent? Am not sure it matters.
Comment 8 Varun Jain 2011-09-06 16:20:24 PDT
Created attachment 106512 [details]
Patch
Comment 9 Robert Kroeger 2011-09-07 10:29:51 PDT
Comment on attachment 106512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106512&action=review

LGTM.

> ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=67645

I think you ought to say why we want this change somewhere.

In particular: it's worth looking at https://bugs.webkit.org/show_bug.cgi?id=65545 and making sure that this code aligns with it.

> Source/WebKit/chromium/tests/InnerGestureRecognizerTest.cpp:292
> +    OwnPtr<Vector<WebCore::PlatformGestureEvent> > gestureStart(gm.processTouchEventForGestures(pressEvent, false));

OwnPtr<...> could have a typedef?

> Source/WebKit/chromium/tests/InnerGestureRecognizerTest.cpp:301
> +        ASSERT_NE(PlatformGestureEvent::TapType, (*gestureMove)[i].type());

this is the scroll case yes?
Comment 10 Varun Jain 2011-09-07 12:15:44 PDT
Created attachment 106620 [details]
Patch
Comment 11 Varun Jain 2011-09-07 12:16:54 PDT
(In reply to comment #9)
> (From update of attachment 106512 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106512&action=review
> 
> LGTM.
> 
> > ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=67645
> 
> I think you ought to say why we want this change somewhere.

Added to the ChangeLog

> 
> In particular: it's worth looking at https://bugs.webkit.org/show_bug.cgi?id=65545 and making sure that this code aligns with it.
> 
> > Source/WebKit/chromium/tests/InnerGestureRecognizerTest.cpp:292
> > +    OwnPtr<Vector<WebCore::PlatformGestureEvent> > gestureStart(gm.processTouchEventForGestures(pressEvent, false));
> 
> OwnPtr<...> could have a typedef?

Done.

> 
> > Source/WebKit/chromium/tests/InnerGestureRecognizerTest.cpp:301
> > +        ASSERT_NE(PlatformGestureEvent::TapType, (*gestureMove)[i].type());
> 
> this is the scroll case yes?

Yes.. but it is tested in gestureScrollTest, so I didnot want to repeat all that code here.
Comment 12 WebKit Review Bot 2011-09-08 11:29:45 PDT
Comment on attachment 106620 [details]
Patch

Clearing flags on attachment: 106620

Committed r94772: <http://trac.webkit.org/changeset/94772>
Comment 13 WebKit Review Bot 2011-09-08 11:29:50 PDT
All reviewed patches have been landed.  Closing bug.