Bug 54417

Created attachment 82375 [details] patch1 A simple implementation of a Chromium platform gesture recognizer with unit tests and layout tests.

Comment on attachment 82375 [details] patch1 patch is obsolete.

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 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 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

(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.

Created attachment 92966 [details] Patch

Comment on attachment 92966 [details] Patch bad patch

Created attachment 92971 [details] Patch

> 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 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.

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()

Benjamin, is some of this code reusable by us when we upstream our code?

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

(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.

Created attachment 94995 [details] Patch

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 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 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 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 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

Created attachment 95182 [details] Patch

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.

Created attachment 95672 [details] Patch

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.

Created attachment 95681 [details] Patch

Comment on attachment 95681 [details] Patch merge conflict. fixed shortly.

Created attachment 95759 [details] Patch

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 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?

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 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 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.

Created attachment 96303 [details] Patch

Comment on attachment 96303 [details] Patch Forwarding benjamin's r+.

Comment on attachment 96303 [details] Patch Clearing flags on attachment: 96303 Committed r88307: <http://trac.webkit.org/changeset/88307>

All reviewed patches have been landed. Closing bug.

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