Bug 49345

Summary: Add Support to WebCore to optionally call an optional platform-specific gesture recognizer
Product: WebKit Reporter: Robert Kroeger <rjkroege>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: benjamin, benm, commit-queue, dglazkov, efidler, gmak, hausmann, kenneth, kim.1.gronholm, manyoso, peter, tonikitoo, webkit-ews, webkit.review.bot, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 53510    
Bug Blocks: 54417    
Attachments:
Description Flags
patch 1 (of probably many) for a basic Chromium GestureManager
none
second proposed patch.
abarth: commit-queue-
third proposed patch
none
fourth patch
tonikitoo: review-, tonikitoo: commit-queue-
fifth patch
rjkroege: review-, rjkroege: commit-queue-
patch6
none
patch7
none
new patch
none
patch
none
updated patch
none
next patch
none
patch
none
patch
none
updated patch
none
patch next
none
Patch
none
Patch
none
Patch none

Description Robert Kroeger 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.
Comment 1 Robert Kroeger 2010-11-10 14:38:41 PST
Created attachment 73542 [details]
patch 1 (of probably many) for a basic Chromium GestureManager
Comment 2 Adam Barth 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)
Comment 3 Robert Kroeger 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.
Comment 4 Robert Kroeger 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.
Comment 5 Adam Barth 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.
Comment 6 Adam Barth 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.
Comment 7 Benjamin Poulain 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.
Comment 8 Robert Kroeger 2010-12-10 15:46:23 PST
Created attachment 76273 [details]
third proposed patch
Comment 9 Robert Kroeger 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.
Comment 10 Robert Kroeger 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.
Comment 11 Robert Kroeger 2010-12-10 16:18:50 PST
Created attachment 76281 [details]
fourth patch
Comment 12 WebKit Review Bot 2010-12-10 16:29:44 PST
Attachment 76273 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6938050
Comment 13 Robert Kroeger 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.
Comment 14 WebKit Review Bot 2010-12-10 17:59:20 PST
Attachment 76281 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6922050
Comment 15 WebKit Review Bot 2010-12-10 18:03:05 PST
Attachment 76281 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7005033
Comment 16 Benjamin Poulain 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.
Comment 17 Antonio Gomes 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(?)
Comment 18 Robert Kroeger 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.
Comment 19 Robert Kroeger 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.
Comment 20 Robert Kroeger 2010-12-16 17:24:58 PST
Created attachment 76833 [details]
fifth patch

Please take another look.
Comment 21 Robert Kroeger 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.
Comment 22 Antonio Gomes 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?
Comment 23 Robert Kroeger 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.
Comment 24 Robert Kroeger 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.
Comment 25 Robert Kroeger 2011-02-09 15:13:12 PST
I have divided this patch up into smaller sections hence the dependency on 53510
Comment 26 Robert Kroeger 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.
Comment 27 Robert Kroeger 2011-02-09 15:43:16 PST
Created attachment 81887 [details]
patch6

Takes a platform specific approach.
Comment 28 WebKit Review Bot 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.
Comment 29 Early Warning System Bot 2011-02-09 16:59:22 PST
Attachment 81887 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7886068
Comment 30 Antonio Gomes 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.
Comment 31 Robert Kroeger 2011-02-10 15:57:35 PST
Created attachment 82066 [details]
patch7

Addressed comments reviewer comments. Please take another look.
Comment 32 Robert Kroeger 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.
Comment 33 Robert Kroeger 2011-02-16 08:58:33 PST
Created attachment 82649 [details]
patch

fixed bad indenting.
Comment 34 Benjamin Poulain 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?
Comment 35 Robert Kroeger 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
Comment 36 Robert Kroeger 2011-02-16 12:20:54 PST
Created attachment 82674 [details]
updated patch

Fixed spelling mistake in comment.
Comment 37 Robert Kroeger 2011-02-16 12:45:07 PST
Created attachment 82677 [details]
next patch

Removed setters/getters that might be useful in the future.
Comment 38 Benjamin Poulain 2011-02-16 12:55:33 PST
Look good to me.
Comment 39 Robert Kroeger 2011-02-17 15:46:56 PST
Created attachment 82871 [details]
patch

Corrected the Changelog.
Comment 40 WebKit Review Bot 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.
Comment 41 Early Warning System Bot 2011-02-17 16:12:09 PST
Attachment 82871 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7920899
Comment 42 Robert Kroeger 2011-02-18 10:30:55 PST
Created attachment 82977 [details]
patch

Fixes the build on qt.
Comment 43 WebKit Review Bot 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.
Comment 44 Antonio Gomes 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();
Comment 45 Robert Kroeger 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.
Comment 46 Robert Kroeger 2011-03-10 13:59:39 PST
Created attachment 85387 [details]
updated patch

Addressed all reviewer comments. Please take another look.
Comment 47 Benjamin Poulain 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.
Comment 48 Robert Kroeger 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.
Comment 49 WebKit Review Bot 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.
Comment 50 Robert Kroeger 2011-03-21 12:35:26 PDT
Comment on attachment 86346 [details]
patch next

oops merge error. new patch in a jiffy.
Comment 51 Robert Kroeger 2011-03-21 13:00:36 PDT
Created attachment 86357 [details]
Patch
Comment 52 Antonio Gomes 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.
Comment 53 Benjamin Poulain 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
Comment 54 Robert Kroeger 2011-03-21 13:52:39 PDT
Created attachment 86360 [details]
Patch
Comment 55 Robert Kroeger 2011-03-21 14:08:33 PDT
Created attachment 86362 [details]
Patch
Comment 56 Antonio Gomes 2011-03-21 14:12:41 PDT
Comment on attachment 86362 [details]
Patch

Looking forward to see it being used! :)
Comment 57 WebKit Commit Bot 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.
Comment 58 WebKit Commit Bot 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>
Comment 59 WebKit Commit Bot 2011-03-21 16:19:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 60 WebKit Commit Bot 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.