[Webkit-unassigned] [Bug 54417] [chromium] Add a basic gesture recognizer to the Chromium platform

Thu May 26 10:08:44 PDT 2011

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

--- Comment #17 from Robert Kroeger <rjkroege at chromium.org>  2011-05-26 10:08:43 PST ---
(From update of attachment 92971)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.