Bug 54417

Summary: [chromium] Add a basic gesture recognizer to the Chromium platform
Product: WebKit Reporter: Robert Kroeger <rjkroege>
Component: WebCore Misc.Assignee: Robert Kroeger <rjkroege>
Status: RESOLVED FIXED    
Severity: Enhancement CC: abarth, benjamin, commit-queue, kenneth, ossy, webkit.review.bot, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
Bug Depends on: 49345    
Bug Blocks: 61550    
Attachments:
Description Flags
patch1
rjkroege: review-, rjkroege: commit-queue-
patch2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Robert Kroeger 2011-02-14 15:30:48 PST
The patch (forthcoming) adds a simple gesture recognizer to Chromium. A more complex (and interesting) gesture recognizer will appear in a subsequent patch.
Comment 1 Robert Kroeger 2011-02-14 15:52:33 PST
Created attachment 82375 [details]
patch1

A simple implementation of a Chromium platform gesture recognizer with unit tests and layout tests.
Comment 2 Robert Kroeger 2011-03-23 15:25:36 PDT
Comment on attachment 82375 [details]
patch1

patch is obsolete.
Comment 3 Robert Kroeger 2011-05-09 13:44:30 PDT
Created attachment 92841 [details]
patch2

Hi Adam, Benjamin,

I was wondering if you could take a look at this patch.

Adam: you've seen older versions of this and I'd thought you might be interested.
Benjamin: this is the follow-on to https://bugs.webkit.org/show_bug.cgi?id=49345.

Please suggest other reviewers as desirable.

This CL contains a framework for gesture recognition and some simple gestures. More complex gestures will follow in a subsequent CL.
Comment 4 Adam Barth 2011-05-09 14:01:47 PDT
Comment on attachment 92841 [details]
patch2

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

I don't really understand gestures, so the below are just general nits.  Also, none of this code seems to be Chromium specific.  Perhaps it shouldn't be Chromium specific?  As an example, WebKit has image decodes that are used by some (but not all) platforms.  This seems like a general platform-agnostic implementation that some ports might want to replace with native gesture managers.  As such, it shouldn't be in Chromium-specific files.

> Source/WebCore/platform/PlatformGestureRecognizer.cpp:39
> +#if !PLATFORM(CHROMIUM)

We probably should exclude this file from the Chromium build instead of adding platform-specific ifdefs to a platform agnostic file.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:40
> +// Edge functions.

I'd remove this comment.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:49
> +// Maximum hold down (s) for a touch to be treated as a click.
> +static const double maxClickDownTime = 0.8;

I'd rename this constant so that the comment is not needed.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:52
> +// Minimum hold down (s) for a touch to be treated as a click.
> +static const double minClickDownTime = 0.01;

ditto

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:56
> +// Maximum manhattan displacement for touch motion to
> +// still be considered as a click.
> +static const int maxManhattanDistance = 20;

ditto

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:66
> +// TODO(rjkroege): write this function

WebKit uses FIXME comments rather than TODO comments.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:173
> +//
> +// Edge functions
> +//

No need for these kinds of comments.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:176
> +    gm->setState(GestureRecognizerChromium::PendingSyntheticClick);

gm => gestureRecongizer

WebKit prefers variable names that are made of complete english words.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:180
> +bool reset(GestureRecognizerChromium* gm, const PlatformTouchPoint& p)

Unused parameters should have their names omitted (otherwise they won't build on PLATFORM(MAC))

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:48
> +// A GestureRecognizerChromium detects gestures occurring in the touch event.
> +// In response to a given touch event, the GestureRecognizerChromium, updates
> +// its internal state and optionally dispatches synthetic events to the
> +// invoking WebViewImpl.

WebKit doesn't usually have class-level comments.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:52
> +    // The states of the gesture manager state machine.

This comment is useless.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:64
> +    // Adds a new edge function to the GestureRecognizerChromium.

Almost all of these comment don't add any information and should be removed.
Comment 5 Robert Kroeger 2011-05-10 08:55:09 PDT
Comment on attachment 92841 [details]
patch2

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

>> Source/WebCore/platform/PlatformGestureRecognizer.cpp:39
>> +#if !PLATFORM(CHROMIUM)
> 
> We probably should exclude this file from the Chromium build instead of adding platform-specific ifdefs to a platform agnostic file.

Done in patch3

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:40
>> +// Edge functions.
> 
> I'd remove this comment.

excessive comments removed in patch3

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:49
>> +static const double maxClickDownTime = 0.8;
> 
> I'd rename this constant so that the comment is not needed.

all fixed in patch3

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:66
>> +// TODO(rjkroege): write this function
> 
> WebKit uses FIXME comments rather than TODO comments.

done. the comment was out of date.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:176
>> +    gm->setState(GestureRecognizerChromium::PendingSyntheticClick);
> 
> gm => gestureRecongizer
> 
> WebKit prefers variable names that are made of complete english words.

done in patch3

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:180
>> +bool reset(GestureRecognizerChromium* gm, const PlatformTouchPoint& p)
> 
> Unused parameters should have their names omitted (otherwise they won't build on PLATFORM(MAC))

done in patch3

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:48
>> +// invoking WebViewImpl.
> 
> WebKit doesn't usually have class-level comments.

removed in patch3

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:52
>> +    // The states of the gesture manager state machine.
> 
> This comment is useless.

removed in patch3

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:64
>> +    // Adds a new edge function to the GestureRecognizerChromium.
> 
> Almost all of these comment don't add any information and should be removed.

removed in patch3
Comment 6 Robert Kroeger 2011-05-10 08:59:34 PDT
(In reply to comment #4)
> (From update of attachment 92841 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92841&action=review
> 
> I don't really understand gestures, so the below are just general nits.  

nits addressed in patch3.

> Also, none of this code seems to be Chromium specific.  Perhaps it shouldn't be Chromium specific?  As an example, WebKit has image decodes that are used by some (but not all) platforms.  This seems like a general platform-agnostic implementation that some ports might want to replace with native gesture managers.  As such, it shouldn't be in Chromium-specific files.

My understanding of review comments from the previous patch in this sequence is that the reviewers were strongly of the opinion that even though this gesture recognizer is platform agnostic, it should remain specific to the Chromium platform until desired by other platforms.

Rob.
Comment 7 Robert Kroeger 2011-05-10 09:23:24 PDT
Created attachment 92966 [details]
Patch
Comment 8 Robert Kroeger 2011-05-10 09:26:05 PDT
Comment on attachment 92966 [details]
Patch

bad patch
Comment 9 Robert Kroeger 2011-05-10 09:58:39 PDT
Created attachment 92971 [details]
Patch
Comment 10 Adam Barth 2011-05-10 14:46:34 PDT
> My understanding of review comments from the previous patch in this sequence is that the reviewers were strongly of the opinion that even though this gesture recognizer is platform agnostic, it should remain specific to the Chromium platform until desired by other platforms.

I'm happy to defer to folks who are more familiar with this topic than I am.  :)
Comment 11 Benjamin Poulain 2011-05-18 10:04:30 PDT
Comment on attachment 92971 [details]
Patch

A synthetic click usually include the mouse move, which will ensure the mouseover/mouseout.

A quick review, mostly minor stuff.
My main concern is the lack of mouse move for synthetic click, that will break some websites.


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

> Source/WebCore/platform/PlatformGestureRecognizer.h:62
> +    // Clears the GestureRecognizer to its initial state. It's necessary to call this before each
> +    // layout test to reduce flakiness.

I think part this comment is overkill: "It's necessary to call this before each layout test to reduce flakiness."
Make sense to me to reset your gesture manager with each page.

> Source/WebCore/platform/PlatformWheelEvent.h:109
> +        PlatformWheelEvent(IntPoint position,

Please also initialize the MAC attribute to there default values, just for completeness.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:85
> +    int manhattanDistance = abs(point.pos().x() - m_firstTouchPosition.x()) +
> +        abs(point.pos().y() - m_firstTouchPosition.y());

Just style: if you really want to split this line, please align the two abs().

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:96
> +void GestureRecognizerChromium::dispatchSyntheticClick(const PlatformTouchPoint& point)
> +{
> +    PlatformMouseEvent fakeMouseDown(point.pos(), point.screenPos(), LeftButton, MouseEventPressed, 1, false, false, false, false, m_lastTouchTime);
> +    PlatformMouseEvent fakeMouseUp(point.pos(), point.screenPos(), LeftButton, MouseEventReleased, 1, false, false, false, false, m_lastTouchTime);
> +
> +    m_eventHandler->handleMousePressEvent(fakeMouseDown);
> +    m_eventHandler->handleMouseReleaseEvent(fakeMouseUp);
> +}

Check this: http://chaos.troll.no/~poulain/touchtesting/click_legacy_events.html
It is the way most touch browser behave (click twice to get the expected results, otherwise the mouseover is in the results).

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:158
> +unsigned int GestureRecognizerChromium::hash(State gestureState, unsigned id, PlatformTouchPoint::State touchType, bool handled)

I wonder why the hash is a member of GestureRecognizerChromium. It does not use any attribute of the object.

It could be a regular function, or a static function of the class.

The name also bother me a little. Because hash() tend to be associated with destructive hash, for which you have have collisions. In this case, a collision would be a bug. So maybe something like "signature()" would be better?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:160
> +    return 1 + ((touchType & 7) | (handled ? 1 << 3 : 0) | ((id & 0xfff) << 4 ) | (gestureState << 16));

Why the +1?
I would use 0x7, just to make it clear it is a mask.

Also, the id can now be arbitrary in the spec. This means someone could want to hash the id itself to avoid people relying on it. It would be nice to have an assertion to make sure the id is < 0xfff. Otherwise someone might break you code by accident in the future.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:210
> +    gestureRecognizer->init();

Any reason not to initialize that in the constructor?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:211
> +    return PassOwnPtr<PlatformGestureRecognizer>(gestureRecognizer);

You should use adoptPtr here.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:219
> +PlatformGestureRecognizer::PlatformGestureRecognizer()
> +{
> +}
> +PlatformGestureRecognizer::~PlatformGestureRecognizer()
> +{
> +}

Wouldn't it be cleaner to include the original cpp file?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:42
> +class GestureRecognizerChromium;

You probably don't need this.
Comment 12 Benjamin Poulain 2011-05-18 10:06:53 PDT
Damn, I messed up the formating.
The line "A synthetic click usually include the mouse move, which will ensure the mouseover/mouseout." was supposed to be in context of GestureRecognizerChromium::dispatchSyntheticClick()
Comment 13 Kenneth Rohde Christiansen 2011-05-18 13:25:42 PDT
Benjamin, is some of this code reusable by us when we upstream our code?
Comment 14 Kenneth Rohde Christiansen 2011-05-18 13:50:41 PDT
Comment on attachment 92971 [details]
Patch

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

A few mini comments when looking thru the patch. Benjamin pointed out the important stuff

> Source/WebCore/platform/PlatformWheelEvent.h:120
> +        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)

we normally keep method signatures on one line - that might even be part of our coding style

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:47
> +static const double maximumTouchDownDurationForClick = 0.8;

Seconds?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:49
> +static const int maximumTouchMoveForClick = 20;

I guess this is pixels? The names can be improved to make this more clear

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:53
> +    , m_state(GestureRecognizerChromium::NoGesture)

I guess NoGesture would be sufficient here?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:71
> +void GestureRecognizerChromium::addEdgeFunction(State state, unsigned finger, PlatformTouchPoint::State touchType, bool handled, GestureTransitionFunction f)

fingerID ?

I dont like that the method is called addEdgeFunction and has a bool of handled... what is handled? the api could be more clear

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:91
> +    PlatformMouseEvent fakeMouseDown(point.pos(), point.screenPos(), LeftButton, MouseEventPressed, 1, false, false, false, false, m_lastTouchTime);

very difficult seeing what those false means. A bit sad that they are not enums or flags... anyway you could add something like /* immediate */ false etc that is done elsewhere in webkit

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:185
> +static bool clickOrScroll(GestureRecognizerChromium* gestureRecognizer, const PlatformTouchPoint& point)

isClickOrScroll?

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:210
>> +    gestureRecognizer->init();
> 
> Any reason not to initialize that in the constructor?

WebKit normally uses initialize() instead of init() - at least last I checked

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:217
> +}
> +PlatformGestureRecognizer::~PlatformGestureRecognizer()

Add a newline between these

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:65
> +    void setState(State s) { m_state = s; }

I would use value instead of s

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:67
> +    State state() { return m_state; }
> +protected:

Add newline before protected:

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:68
> +    void updateValues(double touchTime, const PlatformTouchPoint &);

placement of & is wrong

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:70
> +    unsigned int hash(State, unsigned, PlatformTouchPoint::State, bool);
> +    WTF::HashMap<int, GestureTransitionFunction> m_edgeFunctions;

Add a newline before the hashmap

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:72
> +    double m_firstTouchTime;

time_t?

> Source/WebKit/chromium/tests/GestureRecognizerChromiumTest.cpp:43
> +    InspectableGestureRecognizerChromium() : WebCore::GestureRecognizerChromium() { }

I would add the :... part to the next line

> Source/WebKit/chromium/tests/GestureRecognizerChromiumTest.cpp:48
> +    int tHash(State gestureState,
> +              unsigned id,
> +              PlatformTouchPoint::State touchType,
> +              bool handled)

one line please

> Source/WebKit/chromium/tests/GestureRecognizerChromiumTest.cpp:66
> +    virtual void tUpdateValues(double d, const PlatformTouchPoint &p)

I dont understand this t prefix?

> Source/WebKit/chromium/tests/GestureRecognizerChromiumTest.cpp:80
> +      m_pos.setX(x);
> +      m_screenPos.setX(x);

indentation seems wrong here

> Source/WebKit/chromium/tests/GestureRecognizerChromiumTest.cpp:95
> +    m_screenPos = IntPoint(0, 0);

I belive that we can do IntPoint::zero()

> Source/WebKit/chromium/tests/GestureRecognizerChromiumTest.cpp:152
> +
> +

why two newlines here? and above
Comment 15 Benjamin Poulain 2011-05-18 15:19:49 PDT
(In reply to comment #13)
> Benjamin, is some of this code reusable by us when we upstream our code?

I am afraid not. But we should try. Let's see what we can do.
Comment 16 Robert Kroeger 2011-05-26 10:05:32 PDT
Created attachment 94995 [details]
Patch
Comment 17 Robert Kroeger 2011-05-26 10:08:43 PDT
Comment on attachment 92971 [details]
Patch

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

I have attempted to address all the review comments in the most recent patch. Please have another look.

>> Source/WebCore/platform/PlatformGestureRecognizer.h:62
>> +    // layout test to reduce flakiness.
> 
> I think part this comment is overkill: "It's necessary to call this before each layout test to reduce flakiness."
> Make sense to me to reset your gesture manager with each page.

done in p5

>> Source/WebCore/platform/PlatformWheelEvent.h:109
>> +        PlatformWheelEvent(IntPoint position,
> 
> Please also initialize the MAC attribute to there default values, just for completeness.

done in p5

>> Source/WebCore/platform/PlatformWheelEvent.h:120
>> +                           bool metaKey)
> 
> we normally keep method signatures on one line - that might even be part of our coding style

Done in p5.  (As far as I can tell, there's no mention of this in the coding style. If this is the convention, perhaps there should be?)

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:47
>> +static const double maximumTouchDownDurationForClick = 0.8;
> 
> Seconds?

done in p5

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:49
>> +static const int maximumTouchMoveForClick = 20;
> 
> I guess this is pixels? The names can be improved to make this more clear

done in p5

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:53
>> +    , m_state(GestureRecognizerChromium::NoGesture)
> 
> I guess NoGesture would be sufficient here?

done in p5

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:71
>> +void GestureRecognizerChromium::addEdgeFunction(State state, unsigned finger, PlatformTouchPoint::State touchType, bool handled, GestureTransitionFunction f)
> 
> fingerID ?
> 
> I dont like that the method is called addEdgeFunction and has a bool of handled... what is handled? the api could be more clear

finger -> fingerID in p5

I have made a richer name for handled. Would a comment be desirable/permissible?

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:85
>> +        abs(point.pos().y() - m_firstTouchPosition.y());
> 
> Just style: if you really want to split this line, please align the two abs().

line unsplit in p5

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:91
>> +    PlatformMouseEvent fakeMouseDown(point.pos(), point.screenPos(), LeftButton, MouseEventPressed, 1, false, false, false, false, m_lastTouchTime);
> 
> very difficult seeing what those false means. A bit sad that they are not enums or flags... anyway you could add something like /* immediate */ false etc that is done elsewhere in webkit

I'd prefer not to go changing PlatformMouseEvent in this patch. Maybe later. :-)

added comments to the nums and bools in p5

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:96
>> +}
> 
> Check this: http://chaos.troll.no/~poulain/touchtesting/click_legacy_events.html
> It is the way most touch browser behave (click twice to get the expected results, otherwise the mouseover is in the results).

I've corrected the code and layout tests to pass this test in p5. Thanks for the pointer.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:158
>> +unsigned int GestureRecognizerChromium::hash(State gestureState, unsigned id, PlatformTouchPoint::State touchType, bool handled)
> 
> I wonder why the hash is a member of GestureRecognizerChromium. It does not use any attribute of the object.
> 
> It could be a regular function, or a static function of the class.
> 
> The name also bother me a little. Because hash() tend to be associated with destructive hash, for which you have have collisions. In this case, a collision would be a bug. So maybe something like "signature()" would be better?

made a static function named "signature()" in p5.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:160
>> +    return 1 + ((touchType & 7) | (handled ? 1 << 3 : 0) | ((id & 0xfff) << 4 ) | (gestureState << 16));
> 
> Why the +1?
> I would use 0x7, just to make it clear it is a mask.
> 
> Also, the id can now be arbitrary in the spec. This means someone could want to hash the id itself to avoid people relying on it. It would be nice to have an assertion to make sure the id is < 0xfff. Otherwise someone might break you code by accident in the future.

done in p5

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:185
>> +static bool clickOrScroll(GestureRecognizerChromium* gestureRecognizer, const PlatformTouchPoint& point)
> 
> isClickOrScroll?

done in p5.

>>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:210
>>> +    gestureRecognizer->init();
>> 
>> Any reason not to initialize that in the constructor?
> 
> WebKit normally uses initialize() instead of init() - at least last I checked

init renamed to initialize in p5. My reasoning for splitting the initialize from the constructor was that I wanted initialize to be virtual so it would be easy to subclass this code.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:211
>> +    return PassOwnPtr<PlatformGestureRecognizer>(gestureRecognizer);
> 
> You should use adoptPtr here.

done in p5

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:217
>> +PlatformGestureRecognizer::~PlatformGestureRecognizer()
> 
> Add a newline between these

done in p5

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:219
>> +}
> 
> Wouldn't it be cleaner to include the original cpp file?

Perhaps. But then I'd have to #ifdef out PlatformGestureRecognizer::create there. As it is, PlatformGestureRecogizer.ccp can be #ifdef free. I could split PlatformGestureRecognizer.cpp up to preserve the #ifdef-free property.  What would you recommend?

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:42
>> +class GestureRecognizerChromium;
> 
> You probably don't need this.

done in p5

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:65
>> +    void setState(State s) { m_state = s; }
> 
> I would use value instead of s

done in p5

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:67
>> +protected:
> 
> Add newline before protected:

done in p5

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:68
>> +    void updateValues(double touchTime, const PlatformTouchPoint &);
> 
> placement of & is wrong

done in p5

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:70
>> +    WTF::HashMap<int, GestureTransitionFunction> m_edgeFunctions;
> 
> Add a newline before the hashmap

done in p5

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:72
>> +    double m_firstTouchTime;
> 
> time_t?

Why? This doesn't seem to right to me -- this time comes from the double number of seconds in a PlatformTouchEvent (and as a result can be advanced for the gesture recognizer with DumpRenderTree's EventSender::leapForward.)

>> Source/WebKit/chromium/tests/GestureRecognizerChromiumTest.cpp:43
>> +    InspectableGestureRecognizerChromium() : WebCore::GestureRecognizerChromium() { }
> 
> I would add the :... part to the next line

done, p5

>> Source/WebKit/chromium/tests/GestureRecognizerChromiumTest.cpp:48
>> +              bool handled)
> 
> one line please

done, p5

>> Source/WebKit/chromium/tests/GestureRecognizerChromiumTest.cpp:66
>> +    virtual void tUpdateValues(double d, const PlatformTouchPoint &p)
> 
> I dont understand this t prefix?

fixed in p5

>> Source/WebKit/chromium/tests/GestureRecognizerChromiumTest.cpp:80
>> +      m_screenPos.setX(x);
> 
> indentation seems wrong here

fixed in p5

>> Source/WebKit/chromium/tests/GestureRecognizerChromiumTest.cpp:95
>> +    m_screenPos = IntPoint(0, 0);
> 
> I belive that we can do IntPoint::zero()

fixed in p5

>> Source/WebKit/chromium/tests/GestureRecognizerChromiumTest.cpp:152
>> +
> 
> why two newlines here? and above

fixed in p5
Comment 18 Benjamin Poulain 2011-05-27 07:15:46 PDT
Comment on attachment 94995 [details]
Patch

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

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:55
> +    : m_firstTouchTime(0.0)
> +    , m_state(GestureRecognizerChromium::NoGesture)
> +    , m_lastTouchTime(0.0)
> +    , m_eventHandler(0)

To avoid duplicating this kind of code, you could not initialize anything and just call resetState().
I don't mind the current code, just do as you prefer for this.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:93
> +    PlatformMouseEvent fakeMouseMove(point.pos(), point.screenPos(), NoButton, MouseEventMoved, /* clickCount */ 1, /* shift */ false, /* ctrl */ false, /* alt */ false, /* meta */ false, m_lastTouchTime);
> +    PlatformMouseEvent fakeMouseDown(point.pos(), point.screenPos(), LeftButton, MouseEventPressed, /* clickCount */ 1, /* shift */ false, /* ctrl */ false, /* alt */ false, /* meta */ false, m_lastTouchTime);
> +    // TODO: add a fake motion here.
> +    PlatformMouseEvent fakeMouseUp(point.pos(), point.screenPos(), LeftButton, MouseEventReleased, /* clickCount */ 1, /* shift */ false, /* ctrl */ false, /* alt */ false, /* meta */ false, m_lastTouchTime);

You actually don't have to use false for all the modifiers. The PlatformTouchEvent has the value of shift, ctrl, alt, etc.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:152
> +        m_lastTouchTime = touchTime;

You could set this above the if() since it is updated in both cases... just to make things simpler :)

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:163
> +    return 1 + ((touchType & 0x7) | (handled ? 1 << 3 : 0) | ((id & 0xfff) << 4 ) | (gestureState << 16));

The +1 is suspicious as discussed on IRC :)
Comment 19 Benjamin Poulain 2011-05-27 08:05:05 PDT
Comment on attachment 94995 [details]
Patch

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

Some more quick comments:

> LayoutTests/fast/events/touch/touch-gesture-click.html:17
> +var div = document.createElement("div");
> +div.id = "touchtarget";
> +div.style.width = "100px";
> +div.style.height = "100px";
> +div.style.backgroundColor = "blue";

This is a bit weird.
I would have this target div in the html, and use document.getElementById() to get the element in runTest().

Currently, the span of code between the definition of div and its use is big, which does not help reading this code.

> LayoutTests/fast/events/touch/touch-gesture-click.html:47
> +    // debug('have received: ' + event.type);

This can probably be removed :)

> LayoutTests/fast/events/touch/touch-gesture-click.html:69
> +

Empty line

> LayoutTests/fast/events/touch/touch-gesture-click.html:71
> +        // debug('Received: ' + touchEventsReceived + ' mouse events ');

This can be removed.

> LayoutTests/fast/events/touch/touch-gesture-scroll.html:67
> +var EXPECTED_SCROLLS_TOTAL = 2;

Uppercase with underscore, a bit uncommon.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:129
> +        // The graph is sparse: not all edges have functions.

The place of this comment is a bit weird.
I don't think the comment is necessary.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:140
> +    PlatformWheelEvent syntheticWheelEvent(touchPoint.pos(), touchPoint.screenPos(), deltaX, deltaY, deltaX / static_cast<float>(120), deltaY / static_cast<float>(120), ScrollByPixelWheelEvent, false, false, false, false, false);

Same remark conerning the event key modifiers.
Comment 20 Robert Kroeger 2011-05-27 08:30:55 PDT
Comment on attachment 94995 [details]
Patch

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

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:93
>> +    PlatformMouseEvent fakeMouseUp(point.pos(), point.screenPos(), LeftButton, MouseEventReleased, /* clickCount */ 1, /* shift */ false, /* ctrl */ false, /* alt */ false, /* meta */ false, m_lastTouchTime);
> 
> You actually don't have to use false for all the modifiers. The PlatformTouchEvent has the value of shift, ctrl, alt, etc.

True. Done in p6.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:152
>> +        m_lastTouchTime = touchTime;
> 
> You could set this above the if() since it is updated in both cases... just to make things simpler :)

done, p6

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:163
>> +    return 1 + ((touchType & 0x7) | (handled ? 1 << 3 : 0) | ((id & 0xfff) << 4 ) | (gestureState << 16));
> 
> The +1 is suspicious as discussed on IRC :)

done in p6. Great catch. Thanks.
Comment 21 Robert Kroeger 2011-05-27 09:16:00 PDT
Comment on attachment 94995 [details]
Patch

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

>> LayoutTests/fast/events/touch/touch-gesture-click.html:17
>> +div.style.backgroundColor = "blue";
> 
> This is a bit weird.
> I would have this target div in the html, and use document.getElementById() to get the element in runTest().
> 
> Currently, the span of code between the definition of div and its use is big, which does not help reading this code.

done in p6

>> LayoutTests/fast/events/touch/touch-gesture-click.html:47
>> +    // debug('have received: ' + event.type);
> 
> This can probably be removed :)

done in p6.

>> LayoutTests/fast/events/touch/touch-gesture-click.html:69
>> +
> 
> Empty line

done in p6

>> LayoutTests/fast/events/touch/touch-gesture-click.html:71
>> +        // debug('Received: ' + touchEventsReceived + ' mouse events ');
> 
> This can be removed.

done in p6

>> LayoutTests/fast/events/touch/touch-gesture-scroll.html:67
>> +var EXPECTED_SCROLLS_TOTAL = 2;
> 
> Uppercase with underscore, a bit uncommon.

I was following along with some of the other tests in the directory. changed in p6

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:55
>> +    , m_eventHandler(0)
> 
> To avoid duplicating this kind of code, you could not initialize anything and just call resetState().
> I don't mind the current code, just do as you prefer for this.

left as is.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:129
>> +        // The graph is sparse: not all edges have functions.
> 
> The place of this comment is a bit weird.
> I don't think the comment is necessary.

removed in p6

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:140
>> +    PlatformWheelEvent syntheticWheelEvent(touchPoint.pos(), touchPoint.screenPos(), deltaX, deltaY, deltaX / static_cast<float>(120), deltaY / static_cast<float>(120), ScrollByPixelWheelEvent, false, false, false, false, false);
> 
> Same remark conerning the event key modifiers.

fixed in p6
Comment 22 Robert Kroeger 2011-05-27 09:17:44 PDT
Created attachment 95182 [details]
Patch
Comment 23 Benjamin Poulain 2011-05-27 10:33:16 PDT
Comment on attachment 95182 [details]
Patch

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

Just comments so far, I have not found any bug:

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:55
> +    void addEdgeFunction(State, unsigned finger, PlatformTouchPoint::State, bool touchHandledByJavaScript, GestureTransitionFunction);

Looks like this should be private.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:57
> +    virtual void initialize();

I would get rid of this for now.
You are adding some flexibility here without knowing if you will need it. Better do that in the constructor, and add a virtual function later if you ever need it.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:58
> +    virtual void resetState();

Now that I think of it, it is probably better to renate this reset().
Because you have state() and resetState() already, which have a different semantic than this particular resetState().

Would PlatformGestureRecognizer::reset() make sense?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:60
> +    bool isInClickTimeWindow();
> +    bool isInsideManhattanSquare(const PlatformTouchPoint&);

Those two should probably be private as well. It would be nice if they could be used by the edge function without the need to expose everything from the recognizer.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:64
> +    void setState(State value) { m_state = value; }
> +    State state() { return m_state; }

I can see state() being pretty useful for the class's clients, but setState() seems like a dangerous API to have.

That function is used by the edge functions to set the state of the recognizer. I wonder if passing m_state by reference to those functions could clean the design.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:80
> +    bool m_ctrlKey;
> +    bool m_altKey;
> +    bool m_shiftKey;
> +    bool m_metaKey;

This is a bit ugly.
Wouldn't it be better to pass the PlatformTouchEvent to the edge functions? This would probably also simplify the implementation of pinch.
Comment 24 Robert Kroeger 2011-06-01 15:15:23 PDT
Created attachment 95672 [details]
Patch
Comment 25 Robert Kroeger 2011-06-01 15:17:00 PDT
Comment on attachment 95182 [details]
Patch

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

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:55
>> +    void addEdgeFunction(State, unsigned finger, PlatformTouchPoint::State, bool touchHandledByJavaScript, GestureTransitionFunction);
> 
> Looks like this should be private.

done in p7.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:57
>> +    virtual void initialize();
> 
> I would get rid of this for now.
> You are adding some flexibility here without knowing if you will need it. Better do that in the constructor, and add a virtual function later if you ever need it.

Done in p7.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:58
>> +    virtual void resetState();
> 
> Now that I think of it, it is probably better to renate this reset().
> Because you have state() and resetState() already, which have a different semantic than this particular resetState().
> 
> Would PlatformGestureRecognizer::reset() make sense?

done in p7

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:60
>> +    bool isInsideManhattanSquare(const PlatformTouchPoint&);
> 
> Those two should probably be private as well. It would be nice if they could be used by the edge function without the need to expose everything from the recognizer.

done in p7.   All "internal" functions are available only from an InternalGestureRecognizer that cannot be accessed from "outside" and I pass this to the edge functions.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:64
>> +    State state() { return m_state; }
> 
> I can see state() being pretty useful for the class's clients, but setState() seems like a dangerous API to have.
> 
> That function is used by the edge functions to set the state of the recognizer. I wonder if passing m_state by reference to those functions could clean the design.

I limited access to setState and other semi-internal state methods to only the registered edge functions.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:80
>> +    bool m_metaKey;
> 
> This is a bit ugly.
> Wouldn't it be better to pass the PlatformTouchEvent to the edge functions? This would probably also simplify the implementation of pinch.

It might be. But shouldn't we apply the principle you've applied above and avoid changes that are *probably* desirable to support future code? This was the simplest implementation of the feature.
Comment 26 Robert Kroeger 2011-06-01 15:55:33 PDT
Created attachment 95681 [details]
Patch
Comment 27 Robert Kroeger 2011-06-01 16:01:20 PDT
Comment on attachment 95681 [details]
Patch

merge conflict. fixed shortly.
Comment 28 Robert Kroeger 2011-06-02 06:43:46 PDT
Created attachment 95759 [details]
Patch
Comment 29 Benjamin Poulain 2011-06-07 10:12:37 PDT
Comment on attachment 95759 [details]
Patch

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

Yep, I like it.
I think the test will evolve a bit (kinetic scrolling, frames, block + overflow) but that is a good start.

Some comments:

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:92
> +void InnerGestureRecognizer::addEdgeFunction(State state, unsigned fingerId, PlatformTouchPoint::State touchType, bool touchHandledByJavaScript, GestureTransitionFunction f)
> +{
> +    m_edgeFunctions.add(signature(state, fingerId, touchType, touchHandledByJavaScript), f);

A full variable name would be nice instead of "f".

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:158
> +// 1 LSB bit for signature >= 1

This is a bit unclear.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:48
> +    enum State {
> +      NoGesture,
> +      PendingSyntheticClick,
> +      Scroll
> +    };

Is the indent right here?
Comment 30 Benjamin Poulain 2011-06-07 10:23:10 PDT
Comment on attachment 95759 [details]
Patch

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

Yep, I like it.
I think the test will evolve a bit (kinetic scrolling, frames, block + overflow) but that is a good start.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:92
>> +void InnerGestureRecognizer::addEdgeFunction(State state, unsigned fingerId, PlatformTouchPoint::State touchType, bool touchHandledByJavaScript, GestureTransitionFunction f)
>> +{
>> +    m_edgeFunctions.add(signature(state, fingerId, touchType, touchHandledByJavaScript), f);
> 
> A full variable name would be nice instead of "f".

A full variable name would be nice instead of "f".

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:158
>> +// 1 LSB bit for signature >= 1
> 
> This is a bit unclear.

This is a bit unclear.

>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:48
>> +    enum State {
>> +      NoGesture,
>> +      PendingSyntheticClick,
>> +      Scroll
>> +    };
> 
> Is the indent right here?

Is the indent right here?
Comment 31 WebKit Commit Bot 2011-06-07 11:28:05 PDT
The commit-queue encountered the following flaky tests while processing attachment 95759 [details]:

animations/play-state-suspend.html bug 50959 (authors: cmarrin@apple.com and simon.fraser@apple.com)
java/lc3/JavaObject/JavaObjectToByte-006.html bug 60333 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 32 WebKit Commit Bot 2011-06-07 11:30:08 PDT
Comment on attachment 95759 [details]
Patch

Rejecting attachment 95759 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'land-a..." exit_code: 2

Last 500 characters of output:
/usr/local/git/libexec/git-core/git-svn line 576


Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/
Updating OpenSource
From git://git.webkit.org/WebKit
   09ee877..c123e6f  master     -> origin/master
	M	LayoutTests/platform/chromium/test_expectations.txt
	M	LayoutTests/ChangeLog
r88248 = c123e6f9d114ce8dcc990b249b3fa63d746e9fd6 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/8770714
Comment 33 Robert Kroeger 2011-06-07 14:37:38 PDT
Comment on attachment 95759 [details]
Patch

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

>>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:92
>>> +    m_edgeFunctions.add(signature(state, fingerId, touchType, touchHandledByJavaScript), f);
>> 
>> A full variable name would be nice instead of "f".
> 
> A full variable name would be nice instead of "f".

done in merge conflict patch

>>> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:158
>>> +// 1 LSB bit for signature >= 1
>> 
>> This is a bit unclear.
> 
> This is a bit unclear.

done.

>>> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:48
>>> +    };
>> 
>> Is the indent right here?
> 
> Is the indent right here?

no. fixed.
Comment 34 Robert Kroeger 2011-06-07 14:56:47 PDT
Created attachment 96303 [details]
Patch
Comment 35 Adam Barth 2011-06-07 15:01:43 PDT
Comment on attachment 96303 [details]
Patch

Forwarding benjamin's r+.
Comment 36 WebKit Review Bot 2011-06-07 18:18:56 PDT
Comment on attachment 96303 [details]
Patch

Clearing flags on attachment: 96303

Committed r88307: <http://trac.webkit.org/changeset/88307>
Comment 37 WebKit Review Bot 2011-06-07 18:19:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 38 Csaba Osztrogonác 2011-06-07 23:18:54 PDT
The new tests fail on Qt, and skipped by http://trac.webkit.org/changeset/88327

Here are the diffs if anybody is interested in fixing them:

--- /ramdisk/qt-linux-release/build/layout-test-results/fast/events/touch/touch-gesture-click-expected.txt	2011-06-07 22:38:43.064591025 -0700
+++ /ramdisk/qt-linux-release/build/layout-test-results/fast/events/touch/touch-gesture-click-actual.txt	2011-06-07 22:38:43.064591025 -0700
@@ -3,6 +3,8 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
+have received: 1 touch events
+have received: 2 touch events
 Gesture manager not implemented on this platform.
 PASS successfullyParsed is true
 

--- /ramdisk/qt-linux-release/build/layout-test-results/fast/events/touch/touch-gesture-scroll-expected.txt	2011-06-07 22:38:43.108590977 -0700
+++ /ramdisk/qt-linux-release/build/layout-test-results/fast/events/touch/touch-gesture-scroll-actual.txt	2011-06-07 22:38:43.108590977 -0700
@@ -3,7 +3,12 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-Gesture manager not implemented on this platform or broken
+first gesture
+PASS movingdiv.scrollTop is 0
+FAIL movingdiv.scrollLeft should be 90. Was 0.
+second gesture
+FAIL movingdiv.scrollTop should be 95. Was 0.
+FAIL movingdiv.scrollLeft should be 90. Was 0.
 PASS successfullyParsed is true
 
 TEST COMPLETE