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

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2011-05-18 13:50:41 PST ---
(From update of attachment 92971)
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

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