49345 – Add Support to WebCore to optionally call an optional platform-specific gesture recognizer

RESOLVED FIXED Bug 49345

Add Support to WebCore to optionally call an optional platform-specific gesture recognizer

https://bugs.webkit.org/show_bug.cgi?id=49345

Summary Add Support to WebCore to optionally call an optional platform-specific gestu...

Robert Kroeger

Reported 2010-11-10 14:29:46 PST

The attached patch contains a basic GestureManager for Chromium moved into WebKit per http://codereview.chromium.org/3388013/. The GestureManager provides an extensible framework for synthesizing events from incoming touch events. Also included in this patch are: * minor type alterations to WebTouchEvent and WebTouchPoint that improves readability * unit tests * an implementation of two demonstration gestures: click and scroll. * layout tests for click and scroll.

Attachments
patch 1 (of probably many) for a basic Chromium GestureManager (41.98 KB, patch) 2010-11-10 14:38 PST, Robert Kroeger	no flags	Details Formatted Diff Diff
second proposed patch. (46.11 KB, patch) 2010-12-02 12:21 PST, Robert Kroeger	abarth: commit-queue-	Details Formatted Diff Diff
third proposed patch (44.62 KB, patch) 2010-12-10 15:46 PST, Robert Kroeger	no flags	Details Formatted Diff Diff
fourth patch (44.68 KB, patch) 2010-12-10 16:18 PST, Robert Kroeger	tonikitoo: review- tonikitoo: commit-queue-	Details Formatted Diff Diff
fifth patch (45.35 KB, patch) 2010-12-16 17:24 PST, Robert Kroeger	rjkroege: review- rjkroege: commit-queue-	Details Formatted Diff Diff
patch6 (9.51 KB, patch) 2011-02-09 15:43 PST, Robert Kroeger	no flags	Details Formatted Diff Diff
patch7 (9.48 KB, patch) 2011-02-10 15:57 PST, Robert Kroeger	no flags	Details Formatted Diff Diff
new patch (9.54 KB, patch) 2011-02-14 15:15 PST, Robert Kroeger	no flags	Details Formatted Diff Diff
patch (9.54 KB, patch) 2011-02-16 08:58 PST, Robert Kroeger	no flags	Details Formatted Diff Diff
updated patch (9.54 KB, patch) 2011-02-16 12:20 PST, Robert Kroeger	no flags	Details Formatted Diff Diff
next patch (8.98 KB, patch) 2011-02-16 12:45 PST, Robert Kroeger	no flags	Details Formatted Diff Diff
patch (9.35 KB, patch) 2011-02-17 15:46 PST, Robert Kroeger	no flags	Details Formatted Diff Diff
patch (9.86 KB, patch) 2011-02-18 10:30 PST, Robert Kroeger	no flags	Details Formatted Diff Diff
updated patch (9.33 KB, patch) 2011-03-10 13:59 PST, Robert Kroeger	no flags	Details Formatted Diff Diff
patch next (10.86 KB, patch) 2011-03-21 11:57 PDT, Robert Kroeger	no flags	Details Formatted Diff Diff
Patch (11.57 KB, patch) 2011-03-21 13:00 PDT, Robert Kroeger	no flags	Details Formatted Diff Diff
Patch (11.47 KB, patch) 2011-03-21 13:52 PDT, Robert Kroeger	no flags	Details Formatted Diff Diff
Patch (11.48 KB, patch) 2011-03-21 14:08 PDT, Robert Kroeger	no flags	Details Formatted Diff Diff
Show Obsolete (17) View All Add attachment proposed patch, testcase, etc.

Robert Kroeger

Comment 1 2010-11-10 14:38:41 PST

Created attachment 73542 [details] patch 1 (of probably many) for a basic Chromium GestureManager

Adam Barth

Comment 2 2010-11-11 15:05:39 PST

Comment on attachment 73542 [details] patch 1 (of probably many) for a basic Chromium GestureManager View in context: https://bugs.webkit.org/attachment.cgi?id=73542&action=review > WebKit/chromium/src/GestureManager.h:109 > +#if defined(UNIT_TEST) > +public: > +#else > +protected: > +#endif This is unfortunate. > WebKit/chromium/src/GestureManager.h:139 > + // The WebViewImpl that invoked the GestureManager. > + WebViewImpl* m_WebView; m_webView ^-- lower case (several instances in this file)

Robert Kroeger

Comment 3 2010-11-12 03:29:10 PST

(In reply to comment #2) > (From update of attachment 73542 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73542&action=review > > > WebKit/chromium/src/GestureManager.h:109 > > +#if defined(UNIT_TEST) > > +public: > > +#else > > +protected: > > +#endif > > This is unfortunate. It certainly looks a bit ugly. I could probably get rid of it by sub-classing the GestureManager in the test. > > > WebKit/chromium/src/GestureManager.h:139 > > + // The WebViewImpl that invoked the GestureManager. > > + WebViewImpl* m_WebView; > > m_webView > ^-- lower case (several instances in this file) some of this code use to be in a patch-for-Chromium so it may have other Chromium-style -> WebKit style issues. I'll scrub carefully before the next patch iteration.

Robert Kroeger

Comment 4 2010-12-02 12:21:24 PST

Created attachment 75402 [details] second proposed patch. As per discussion earlier, I have moved the GestureManager to WebCore. Please take another look.

Adam Barth

Comment 5 2010-12-07 10:09:43 PST

Comment on attachment 75402 [details] second proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=75402&action=review > LayoutTests/fast/events/touch/touch-gesture-scroll.html:46 > +<body style="margin:0" onload="setTimeout('runTest();', 100)"> Why wait 100ms? > LayoutTests/fast/events/touch/touch-gesture-scroll.html:84 > + successfullyParsed = true; > + isSuccessfullyParsed(); Seems like we should do these even if there's no layoutTestController. > LayoutTests/fast/events/touch/touch-gesture-scroll.html:113 > + // Wait for layout. > + setTimeout(checkScrollOffset, 50); Boo. setTimeout makes tests slow and flaky. > LayoutTests/fast/events/touch/touch-gesture-scroll.html:139 > + layoutTestController.notifyDone(); This should be protected by if (window.layoutTestController) > LayoutTests/fast/events/touch/touch-gesture-scroll.html:155 > + setTimeout(exitIfNecessary, 200); boo > WebCore/page/EventHandler.cpp:3001 > -} > +} // namespace WebCore This change isn't really necessary. We're inconsistent about whether to include these comments. > WebCore/page/GestureManager.cpp:34 > +// #include <wtf/Assertions.h> No commented out code, please. :) > WebCore/page/GestureManager.cpp:53 > +static const double maxClickDownTime = .8; 0.8 > WebCore/page/GestureManager.cpp:56 > +static const double minClickDownTime = .01; ditto > WebCore/page/GestureManager.cpp:78 > +void GestureManager::addEdgeFunction(State state, > + unsigned finger, > + PlatformTouchPoint::State touchType, > + bool handled, > + GestureTransitionFunction f) All on one line please. > WebCore/page/GestureManager.cpp:108 > + PlatformMouseEvent fakeMouseDown(p.pos(), > + p.screenPos(), > + LeftButton, > + MouseEventPressed, > + 1, > + false, > + false, > + false, > + false, > + WTF::currentTime()); WebKit doesn't have an 80 col limit. Please feel free to use more horizontal space. :) > WebCore/page/GestureManager.cpp:153 > + GestureTransitionFunction ef = m_edgeFunctions.get(hash(m_state, p.id(), p.state(), handled)); > + if (ef) We usually combine these lines.

Adam Barth

Comment 6 2010-12-07 13:53:07 PST

Comment on attachment 75402 [details] second proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=75402&action=review My review comments are mostly just style issues. I don't know too much about touch, so I'm not able to review the substance of your change. We'll want to find someone who's familiar with this kind of thing to do a review of the substance. > WebCore/page/GestureManager.h:68 > + void addEdgeFunction(State, > + unsigned, > + PlatformTouchPoint::State, > + bool, > + GestureTransitionFunction); > + Generally, we omit formal parameter names in header files if the meaning is obvious. For example "Frame* frame" is kind of redundant. However, here, it looks like you've gone a bit overboard. What does the second parameter mean? Also, the comment seems to refer to parameter names that are no longer present. (Also, these declarations should be on one line.) > WebCore/page/GestureManager.h:95 > + inline void setState(State s) > + { > + m_state = s; > + } No need to declare these functions inline. Also, you can just write the whole function on one line. (You've also got a bad indent here, but that will get fixed when you put the whole thing on one line.) The general rule is that simple setter/getter functions should take up as little space as possible.

Benjamin Poulain

Comment 7 2010-12-10 14:44:10 PST

Comment on attachment 75402 [details] second proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=75402&action=review > WebCore/platform/qt/PlatformTouchEventQt.cpp:45 > + m_timestamp = 0; Wouldn't it make sense to set timestamp to WTF::currentTime() instead? This way it could be used with your algoritms of GestureManager. I like the patch quite a bit, but I am concerned about platform compatibility. What about forwarding the gestures to the ChromeClient and letting the port decide how to handle the result? I think about features like kinetic scrolling, and zoom behavior which depends on the platform. I am also a bit concerned about putting behavior here when the platform already provide gesture recognition. Windows 7 for example has precise timing for gestures (WM_GESTURE messages), and users get used to the platform recognizer. By giving other timing specifically for WebKit, the engine will be less integrated in the platform.

Robert Kroeger

Comment 8 2010-12-10 15:46:23 PST

Created attachment 76273 [details] third proposed patch

Robert Kroeger

Comment 9 2010-12-10 15:49:22 PST

(In reply to comment #6) > (From update of attachment 75402 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75402&action=review > > My review comments are mostly just style issues. I don't know too much about touch, so I'm not able to review the substance of your change. We'll want to find someone who's familiar with this kind of thing to do a review of the substance. Sure. Sounds good. I am (at least in theory) one of the "touch people" for Chrome but can stand to learn a lot about how touch works in WebKit. > > > WebCore/page/GestureManager.h:68 > > + void addEdgeFunction(State, > > + unsigned, > > + PlatformTouchPoint::State, > > + bool, > > + GestureTransitionFunction); > > + > > Generally, we omit formal parameter names in header files if the meaning is obvious. For example "Frame* frame" is kind of redundant. However, here, it looks like you've gone a bit overboard. What does the second parameter mean? Also, the comment seems to refer to parameter names that are no longer present. (Also, these declarations should be on one line.) I cleaned this up. > > > WebCore/page/GestureManager.h:95 > > + inline void setState(State s) > > + { > > + m_state = s; > > + } > > No need to declare these functions inline. Also, you can just write the whole function on one line. (You've also got a bad indent here, but that will get fixed when you put the whole thing on one line.) The general rule is that simple setter/getter functions should take up as little space as possible. done.

Robert Kroeger

Comment 10 2010-12-10 16:18:09 PST

(In reply to comment #5) > (From update of attachment 75402 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75402&action=review > > > LayoutTests/fast/events/touch/touch-gesture-scroll.html:46 > > +<body style="margin:0" onload="setTimeout('runTest();', 100)"> > > Why wait 100ms? because EventSender::sendCurrentTouchEvent didn't call webview()->layout(). fixed. > > > LayoutTests/fast/events/touch/touch-gesture-scroll.html:84 > > + successfullyParsed = true; > > + isSuccessfullyParsed(); > > Seems like we should do these even if there's no layoutTestController. done. > > > LayoutTests/fast/events/touch/touch-gesture-scroll.html:113 > > + // Wait for layout. > > + setTimeout(checkScrollOffset, 50); > > Boo. setTimeout makes tests slow and flaky. setTimeout removed throughout. I had forgotten to clean up after https://bugs.webkit.org/show_bug.cgi?id=49285 done. > > > LayoutTests/fast/events/touch/touch-gesture-scroll.html:139 > > + layoutTestController.notifyDone(); > > This should be protected by if (window.layoutTestController) done, fourth patch. > > > LayoutTests/fast/events/touch/touch-gesture-scroll.html:155 > > + setTimeout(exitIfNecessary, 200); > > boo fixed. > > > WebCore/page/EventHandler.cpp:3001 > > -} > > +} // namespace WebCore > > This change isn't really necessary. We're inconsistent about whether to include these comments. done > > > WebCore/page/GestureManager.cpp:34 > > +// #include <wtf/Assertions.h> > > No commented out code, please. :) done. > > > WebCore/page/GestureManager.cpp:53 > > +static const double maxClickDownTime = .8; > > 0.8 > > > WebCore/page/GestureManager.cpp:56 > > +static const double minClickDownTime = .01; > > ditto done > > > WebCore/page/GestureManager.cpp:78 > > +void GestureManager::addEdgeFunction(State state, > > + unsigned finger, > > + PlatformTouchPoint::State touchType, > > + bool handled, > > + GestureTransitionFunction f) > > All on one line please. done. > > > WebCore/page/GestureManager.cpp:108 > > + PlatformMouseEvent fakeMouseDown(p.pos(), > > + p.screenPos(), > > + LeftButton, > > + MouseEventPressed, > > + 1, > > + false, > > + false, > > + false, > > + false, > > + WTF::currentTime()); > > WebKit doesn't have an 80 col limit. Please feel free to use more horizontal space. :) google3 habits. :-) my bad. fixed. > > > WebCore/page/GestureManager.cpp:153 > > + GestureTransitionFunction ef = m_edgeFunctions.get(hash(m_state, p.id(), p.state(), handled)); > > + if (ef) > > We usually combine these lines. done.

Robert Kroeger

Comment 11 2010-12-10 16:18:50 PST

Created attachment 76281 [details] fourth patch

WebKit Review Bot

Comment 12 2010-12-10 16:29:44 PST

Attachment 76273 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6938050

Robert Kroeger

Comment 13 2010-12-10 16:38:26 PST

(In reply to comment #7) > (From update of attachment 75402 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75402&action=review > > > WebCore/platform/qt/PlatformTouchEventQt.cpp:45 > > + m_timestamp = 0; > > Wouldn't it make sense to set timestamp to WTF::currentTime() instead? This way it could be used with your algoritms of GestureManager. yes. fixed. > > > I like the patch quite a bit, but I am concerned about platform compatibility. Thanks. My premise is that touch events, if not handled by the page explicitly, are converted into synthetic events handled by EventSender. Consequently, having a GestureManager "near" the EventSender seemed logical. > > What about forwarding the gestures to the ChromeClient and letting the port decide how to handle the result? > I think about features like kinetic scrolling, and zoom behavior which depends on the platform. This is an excellent suggestion. However, I think that the basic framework here does not preclude your suggestion. I had imagined extending the GestureManager via sub-classes in a future patch. (You will note that a number of popular gestures are not admitted here.) It would make sense to permit the platform layer to provide platform-specific GestureManager extensions. In particular, the framework here permits a future platform-specific implementation to register arbitrary additional edge functions with minimal modifications here. (fingers crossed.) > > I am also a bit concerned about putting behavior here when the platform already provide gesture recognition. Windows 7 for example has precise timing for gestures (WM_GESTURE messages), and users get used to the platform recognizer. By giving other timing specifically for WebKit, the engine will be less integrated in the platform. The goal of this patch was to provide a basic framework that would be used on platforms that do not provide a gesture management system (as like double-click on some cases.) I'd like to defer some sort of platform delegation scheme to a follow-on CL.

WebKit Review Bot

Comment 14 2010-12-10 17:59:20 PST

Attachment 76281 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6922050

WebKit Review Bot

Comment 15 2010-12-10 18:03:05 PST

Attachment 76281 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7005033

Benjamin Poulain

Comment 16 2010-12-11 03:55:44 PST

Comment on attachment 76281 [details] fourth patch Comments from a quick look at the patch: View in context: https://bugs.webkit.org/attachment.cgi?id=76281&action=review > LayoutTests/fast/events/touch/touch-gesture-click.html:21 > +var EXPECTED_CLICK_EVENTS_TOTAL = 3; I would rename this to expected _mouse_ events. Because only one click event is expected so the name can be confusing. > LayoutTests/fast/events/touch/touch-gesture-click.html:25 > +var haveTouchEvents = false; This does not seem to be used anywhere. > LayoutTests/fast/events/touch/touch-gesture-click.html:30 > + lastEvent = event; You could get rid of lastEvent entirely and use event directly here. > LayoutTests/fast/events/touch/touch-gesture-scroll.html:66 > +var movingdiv; This does not need to be global, you are getting the element directly in the functions. > LayoutTests/fast/events/touch/touch-gesture-scroll.html:159 > + if (eventSender.clearTouchPoints) { > + firstTouchDragSequence(); > + } else { > + exitIfNecessary(); > + } No brackets here according to the coding convention? (no idea what our conventions are for JavaScript :)) > WebCore/page/EventHandler.cpp:2989 > + m_gestureManager->processTouchEventForGesture(event, this, defaultPrevented); I guess this should be something like: if (!defaultPrevented) defaultPrevented = m_gestureManager->processTouchEventForGesture(event, this); You pass defaultPrevented in the gesture manager but I did not see used anywhere. If the page is handling the touch event, I would not add a WebKit gesture on top (e.g.: you don't want to scroll the frame when panning the touch version of Google maps). You also need to change the value of defaultPrevented if processTouchEventForGesture does something with the event. If not, platforms usually pass the event to the parent widget. > WebCore/page/GestureManager.cpp:62 > + , m_eventHandler(0) I think this should be passed as an argument of the constructor. There is a relation 1<->1 between the EventHandler and the GestureManager. I don't see the point of passing it in GestureManager::processTouchEventForGesture(). > WebCore/page/GestureManager.cpp:217 > +// To create a richer gesture recognizer, it is sufficient to modify this > +// factory to add additional edges. (Though additional edges may require > +// additional state tracking.) > +GestureManager* createGestureManager() > +{ > + GestureManager* gm = new GestureManager(); > + gm->init(); > + return gm; > +} I am not sure why you need this instead of calling init from the constructor. If subclassing the GestureManager is the way to extend it, adding additional edges could be done in the constructor of the subclass.

Antonio Gomes

Comment 17 2010-12-15 12:29:05 PST

Comment on attachment 76281 [details] fourth patch View in context: https://bugs.webkit.org/attachment.cgi?id=76281&action=review I like Benjamin's review. His comments should be addressed. > WebCore/page/GestureManager.cpp:46 > +// Edge functions. > +static bool click(GestureManager*, const PlatformTouchPoint&); > +static bool clickOrScroll(GestureManager*, const PlatformTouchPoint&); > +static bool inScroll(GestureManager*, const PlatformTouchPoint&); > +static bool reset(GestureManager*, const PlatformTouchPoint&); > +static bool touchDown(GestureManager*, const PlatformTouchPoint&); > + If you are passing the GM here anyways, why not putting these methods as class methods? > WebCore/platform/PlatformWheelEvent.h:114 > > + PlatformWheelEvent(IntPoint position, > + IntPoint globalPosition, > + float deltaX, > + float deltaY, > + float wheelTicksX, > + float wheelTicksY, > + PlatformWheelEventGranularity granularity, > + bool isAccepted, > + bool shiftKey, > + bool ctrlKey, > + bool altKey, > + bool metaKey) > + : m_position(position) > + , m_globalPosition(globalPosition) > + , m_deltaX(deltaX) > + , m_deltaY(deltaY) > + , m_wheelTicksX(wheelTicksX) > + , m_wheelTicksY(wheelTicksY) > + , m_granularity(granularity) > + , m_isAccepted(isAccepted) > + , m_shiftKey(shiftKey) > + , m_ctrlKey(ctrlKey) > + , m_altKey(altKey) > + , m_metaKey(metaKey) > + { > + } You should say in the changelog why you are adding this. (Or it could come on its own patch(?)

Robert Kroeger

Comment 18 2010-12-16 15:31:12 PST

(In reply to comment #16) View in context: https://bugs.webkit.org/attachment.cgi?id=76281&action=review >> LayoutTests/fast/events/touch/touch-gesture-click.html:21 >> +var EXPECTED_CLICK_EVENTS_TOTAL = 3; > > I would rename this to expected _mouse_ events. Because only one click event is expected so the name can be confusing. done, next patch. >> LayoutTests/fast/events/touch/touch-gesture-click.html:25 >> +var haveTouchEvents = false; > > This does not seem to be used anywhere. removed, next patch. >> LayoutTests/fast/events/touch/touch-gesture-click.html:30 >> + lastEvent = event; > > You could get rid of lastEvent entirely and use event directly here. done, next patch. >> LayoutTests/fast/events/touch/touch-gesture-scroll.html:66 >> +var movingdiv; > > This does not need to be global, you are getting the element directly in the functions. It is not obvious to me how to do this. The shouldBe function eval's its args so a non-global movingdiv doesn't end up in scope. I could construct String(movingdiv.scrollTop) as an arg to shouldBe but then the output from a failed test is less helpful. Do you have any suggestions? >> LayoutTests/fast/events/touch/touch-gesture-scroll.html:159 >> + } > > No brackets here according to the coding convention? (no idea what our conventions are for JavaScript :)) done, np >> WebCore/page/EventHandler.cpp:2989 >> + m_gestureManager->processTouchEventForGesture(event, this, defaultPrevented); > > I guess this should be something like: > if (!defaultPrevented) > defaultPrevented = m_gestureManager->processTouchEventForGesture(event, this); > > You pass defaultPrevented in the gesture manager but I did not see used anywhere. If the page is handling the touch event, I would not add a WebKit gesture on top (e.g.: you don't want to scroll the frame when panning the touch version of Google maps). > > You also need to change the value of defaultPrevented if processTouchEventForGesture does something with the event. If not, platforms usually pass the event to the parent widget. I am not convinced that I should update defaultPrevented -- if embedder code relies on knowing that a touch has not been handled in page, then changing defaultPrevented to indicate if a synthetic event (like a click) was generated strikes me as wrong. I pass defaultPrevented to the GM because I wanted the GM to be able to inspect all events -- for example if a future GM instance wanted to maintain a spline representation of an entire touch track, it should not be missing motion events that are being suppressed by the JS code. The defaultPrevented value is used in processTouchEventForGesture -- it is one of the values used to dispatch gesture functions. The set of example gestures in this CL here require it to be false -- but additional gestures in the future will not. >> WebCore/page/GestureManager.cpp:62 >> + , m_eventHandler(0) > > I think this should be passed as an argument of the constructor. There is a relation 1<->1 between the EventHandler and the GestureManager. I don't see the point of passing it in GestureManager::processTouchEventForGesture(). done, np. FWIW: I wrote it this way to make it easier to not have the 1-1 relation. >> WebCore/page/GestureManager.cpp:217 >> +} > > I am not sure why you need this instead of calling init from the constructor. > If subclassing the GestureManager is the way to extend it, adding additional edges could be done in the constructor of the subclass. I think it is easier to re-use the GM as written if there is a constructor that produces an "empty" GM instance and then adds the gesture functions separately. That way, different gestures can be loaded into the GM without needing to modify the class or remove the existing gestures.

Robert Kroeger

Comment 19 2010-12-16 16:49:34 PST

Comment on attachment 76281 [details] fourth patch View in context: https://bugs.webkit.org/attachment.cgi?id=76281&action=review >> WebCore/page/GestureManager.cpp:46 >> + > > If you are passing the GM here anyways, why not putting these methods as class methods? I chose to do this so that gesture functions could be added to this GM implementation without needing to modify it. In particular, platform or embedder code can add additional gestures given this structure. >> WebCore/platform/PlatformWheelEvent.h:114 >> + } > > You should say in the changelog why you are adding this. (Or it could come on its own patch(?) added in the next patch.

Robert Kroeger

Comment 20 2010-12-16 17:24:58 PST

Created attachment 76833 [details] fifth patch Please take another look.

Robert Kroeger

Comment 21 2011-01-05 08:11:37 PST

Reviewers, With the end-of-year holiday time largely behind us, would it be possible to take another look at my revised patch & comments? I'm eager to complete an initial version of this functionality so would appreciate feedback that will help me get the patch into a committable state.

Antonio Gomes

Comment 22 2011-01-13 11:10:34 PST

Comment on attachment 76833 [details] fifth patch View in context: https://bugs.webkit.org/attachment.cgi?id=76833&action=review > WebCore/page/GestureManager.cpp:179 > +static bool click(GestureManager* gm, const PlatformTouchPoint& p) > +{ Could you tell me from where and when this method would be called, for example?

Robert Kroeger

Comment 23 2011-01-18 08:07:17 PST

(In reply to comment #22) > (From update of attachment 76833 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76833&action=review > > > WebCore/page/GestureManager.cpp:179 > > +static bool click(GestureManager* gm, const PlatformTouchPoint& p) > > +{ > > Could you tell me from where and when this method would be called, for example? These gesture functions are called by processTouchEventForGesture. The gesture functions correspond to specific edges in a FSM stored in the m_edgeFunctions HashMap. The FSM is built in GestureManager::init. Having the FSM be dynamic permit modifying it dynamically. In particular, I intend to add functionality in a future patch that will permit the embedder to modify the FSM at runtime. For example, I want it to be possible for the embedder to configure the FSM differently depending on the input devices on the user's computer.

Robert Kroeger

Comment 24 2011-02-03 10:01:24 PST

Comment on attachment 76833 [details] fifth patch I have a new patch in preparation that obsoletes this one.

Robert Kroeger

Comment 25 2011-02-09 15:13:12 PST

I have divided this patch up into smaller sections hence the dependency on 53510

Robert Kroeger

Comment 26 2011-02-09 15:20:37 PST

Discussion with tonikitoo suggested that this bug was mis-named so, now that in patch 6 where I have altered the code (per review advice from Benjamin Poulain below) to permit each platform to provide its own gesture recognizer, I have given this patch a more descriptive name.

Robert Kroeger

Comment 27 2011-02-09 15:43:16 PST

Created attachment 81887 [details] patch6 Takes a platform specific approach.

WebKit Review Bot

Comment 28 2011-02-09 15:47:48 PST

Attachment 81887 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/PlatformGestureRecognizer.h:54: The parameter name "event" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Early Warning System Bot

Comment 29 2011-02-09 16:59:22 PST

Attachment 81887 [details] did not build on qt: Build output: http://queues.webkit.org/results/7886068

Antonio Gomes

Comment 30 2011-02-09 20:52:31 PST

Comment on attachment 81887 [details] patch6 View in context: https://bugs.webkit.org/attachment.cgi?id=81887&action=review > Source/WebCore/ChangeLog:4 > + specific gesture recognizer. A single-owner gesture reccognizer can be typo: cc > Source/WebCore/page/EventHandler.h:231 > + // Use thse to move the GestureRecognizer instance between EventHandlers. typo THSE > Source/WebCore/platform/PlatformGestureRecognizer.h:46 > + PlatformGestureRecognizer(); generally when you have ::create methods (i.e. factory), the ctor is private.

Robert Kroeger

Comment 31 2011-02-10 15:57:35 PST

Created attachment 82066 [details] patch7 Addressed comments reviewer comments. Please take another look.

Robert Kroeger

Comment 32 2011-02-14 15:15:16 PST

Created attachment 82370 [details] new patch Made the constructor protected so that it could be invoked from the subclass.

Robert Kroeger

Comment 33 2011-02-16 08:58:33 PST

Created attachment 82649 [details] patch fixed bad indenting.

Benjamin Poulain

Comment 34 2011-02-16 09:03:18 PST

Comment on attachment 82649 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=82649&action=review > Source/WebCore/page/EventHandler.cpp:3063 > +PassOwnPtr<PlatformGestureRecognizer> EventHandler::gestureRecognizer() > +{ > + return m_gestureRecognizer.release(); > +} > + > +void EventHandler::setGestureRecognizer(PassOwnPtr<PlatformGestureRecognizer> g) > +{ > + m_gestureRecognizer = g; > +} Why do we need getters and setters AND createPlatformGestureRecognizer(). Do you have use case to change the recognizer at runtime? > Source/WebCore/platform/PlatformGestureRecognizer.cpp:40 > +// here that meets its needs. EventSender will ignore null GestureRecognizers. EventSender?

Robert Kroeger

Comment 35 2011-02-16 12:18:44 PST

Comment on attachment 82649 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=82649&action=review >> Source/WebCore/page/EventHandler.cpp:3063 >> +} > > Why do we need getters and setters AND createPlatformGestureRecognizer(). Do you have use case to change the recognizer at runtime? Yes. An embedder might want to temporarily take the gesture recognizer away from the EventHandler for handling touch events on a different thread while performing a latency-critical gesture like a page zoom. (For example, such a feature is being considered for platform/chromium.) >> Source/WebCore/platform/PlatformGestureRecognizer.cpp:40 > > EventSender? EventHandler. Fixed in the next patch, ~10m

Robert Kroeger

Comment 36 2011-02-16 12:20:54 PST

Created attachment 82674 [details] updated patch Fixed spelling mistake in comment.

Robert Kroeger

Comment 37 2011-02-16 12:45:07 PST

Created attachment 82677 [details] next patch Removed setters/getters that might be useful in the future.

Benjamin Poulain

Comment 38 2011-02-16 12:55:33 PST

Look good to me.

Robert Kroeger

Comment 39 2011-02-17 15:46:56 PST

Created attachment 82871 [details] patch Corrected the Changelog.

WebKit Review Bot

Comment 40 2011-02-17 15:51:51 PST

Attachment 82871 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/PlatformGestureRecognizer.h:56: The parameter name "event" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Early Warning System Bot

Comment 41 2011-02-17 16:12:09 PST

Attachment 82871 [details] did not build on qt: Build output: http://queues.webkit.org/results/7920899

Robert Kroeger

Comment 42 2011-02-18 10:30:55 PST

Created attachment 82977 [details] patch Fixes the build on qt.

WebKit Review Bot

Comment 43 2011-02-18 10:33:07 PST

Attachment 82977 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/PlatformGestureRecognizer.h:56: The parameter name "event" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Antonio Gomes

Comment 44 2011-02-23 20:47:35 PST

Comment on attachment 82977 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=82977&action=review Looks good to me too. > Source/WebCore/ChangeLog:5 > + Add Support to WebCore to optionally call a platform-specific gesture recognizer Support -> support >> Source/WebCore/platform/PlatformGestureRecognizer.h:56 >> + virtual bool processTouchEventForGesture(const PlatformTouchEvent& event, EventHandler* source, bool handled) = 0; > > The parameter name "event" adds no information, so it should be removed. [readability/parameter_name] [5] Please remove "event" as per style bot advice. > Source/WebCore/platform/PlatformGestureRecognizer.h:60 > +PlatformGestureRecognizer* createPlatformGestureRecognizer(); I prefer a static class method. static PlatformGestureRecognizer* PlatformGestureRecognizer::create();

Robert Kroeger

Comment 45 2011-03-10 13:58:11 PST

Comment on attachment 82977 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=82977&action=review >> Source/WebCore/ChangeLog:5 >> + Add Support to WebCore to optionally call a platform-specific gesture recognizer > > Support -> support Done >>> Source/WebCore/platform/PlatformGestureRecognizer.h:56 >>> + virtual bool processTouchEventForGesture(const PlatformTouchEvent& event, EventHandler* source, bool handled) = 0; >> >> The parameter name "event" adds no information, so it should be removed. [readability/parameter_name] [5] > > Please remove "event" as per style bot advice. Done. >> Source/WebCore/platform/PlatformGestureRecognizer.h:60 >> +PlatformGestureRecognizer* createPlatformGestureRecognizer(); > > I prefer a static class method. static PlatformGestureRecognizer* PlatformGestureRecognizer::create(); Done.

Robert Kroeger

Comment 46 2011-03-10 13:59:39 PST

Created attachment 85387 [details] updated patch Addressed all reviewer comments. Please take another look.

Benjamin Poulain

Comment 47 2011-03-14 12:49:29 PDT

Comment on attachment 85387 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=85387&action=review My comments: -I would have kept the ENABLE_GESTURE_MANAGER. This is experimental feature at the moment since you don't have the full implementation ready. I would guard whatever goes in WebCore for now. If this WebCore gesture recognition end up being the default for touch event handling, the ENABLE_GESTURE_MANAGER can be removed. If adding the gesture support in WebCore end up being impractical in the end, then we can remove the code guarded by ENABLE_GESTURE_MANAGER. So for now I like the #define (maybe that has already been discussed and I missed the thread?) -Other ports than chromium and Qt are defining ENABLE_TOUCH_EVENTS (at least EFL, and probably WinCE). Those won't build now since EventHandler reference PlatformGestureRecognizer. > Source/WebCore/page/EventHandler.h:38 > +#include <wtf/PassOwnPtr.h> Probably not necessary.

Robert Kroeger

Comment 48 2011-03-21 11:57:45 PDT

Created attachment 86346 [details] patch next Wrapped the GestureRecognizer functionality with an #ifdef per Ben's suggestion and removed a superfluous include.

WebKit Review Bot

Comment 49 2011-03-21 12:00:57 PDT

Attachment 86346 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/WebCore/platform/PlatformGestureRecognizer.h:56: The parameter name "event" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Robert Kroeger

Comment 50 2011-03-21 12:35:26 PDT

Comment on attachment 86346 [details] patch next oops merge error. new patch in a jiffy.

Robert Kroeger

Comment 51 2011-03-21 13:00:36 PDT

Created attachment 86357 [details] Patch

Antonio Gomes

Comment 52 2011-03-21 13:12:42 PDT

Comment on attachment 86357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86357&action=review > Source/WebCore/platform/PlatformGestureRecognizer.cpp:41 > +PlatformGestureRecognizer* PlatformGestureRecognizer::createPlatformGestureRecognizer() just ::create() should be ok too.

Benjamin Poulain

Comment 53 2011-03-21 13:23:26 PDT

Comment on attachment 86357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86357&action=review I was going to r+, cq-, but I see Antonio already did :) > Source/JavaScriptCore/wtf/Platform.h:964 > +#if !defined(ENABLE_GESTURE_RECOGNZER) Missing a I here: recognizer

Robert Kroeger

Comment 54 2011-03-21 13:52:39 PDT

Created attachment 86360 [details] Patch

Robert Kroeger

Comment 55 2011-03-21 14:08:33 PDT

Created attachment 86362 [details] Patch

Antonio Gomes

Comment 56 2011-03-21 14:12:41 PDT

Comment on attachment 86362 [details] Patch Looking forward to see it being used! :)

WebKit Commit Bot

Comment 57 2011-03-21 16:16:21 PDT

The commit-queue encountered the following flaky tests while processing attachment 86362 [details]: http/tests/navigation/changing-frame-hierarchy-in-onload.html bug 56779 (authors: andersca@apple.com and kinuko@chromium.org) The commit-queue is continuing to process your patch.

WebKit Commit Bot

Comment 58 2011-03-21 16:19:32 PDT

Comment on attachment 86362 [details] Patch Clearing flags on attachment: 86362 Committed r81618: <http://trac.webkit.org/changeset/81618>

WebKit Commit Bot

Comment 59 2011-03-21 16:19:39 PDT

All reviewed patches have been landed. Closing bug.

WebKit Commit Bot

Comment 60 2011-03-21 17:52:25 PDT

The commit-queue encountered the following flaky tests while processing attachment 86362 [details]: svg/dom/SVGScriptElement/script-set-href.svg bug 56790 (authors: mitz@webkit.org and zimmermann@kde.org) The commit-queue is continuing to process your patch.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Enhancement

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component WebKit Misc.

Assignee

Nobody

Reported

2010-11-10 14:29 PST

Modified

2011-05-09 11:58 PDT History

CC List

15 users Show

URL

Keywords

Depends on

53510

Blocks

54417

Dependancies

tree graph