Summary: | Split Tap gesture detection into TapDown and TapRelease | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Varun Jain <varunjain> | ||||||||||
Component: | New Bugs | Assignee: | Varun Jain <varunjain> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, rjkroege, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 67709 | ||||||||||||
Attachments: |
|
Description
Varun Jain
2011-09-06 07:34:19 PDT
Created attachment 106416 [details]
Patch
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 on attachment 106416 [details] Patch Attachment 106416 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9592855 (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 Created attachment 106420 [details]
Patch
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. (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. Created attachment 106512 [details]
Patch
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? Created attachment 106620 [details]
Patch
(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 on attachment 106620 [details] Patch Clearing flags on attachment: 106620 Committed r94772: <http://trac.webkit.org/changeset/94772> All reviewed patches have been landed. Closing bug. |