[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 #23 from Benjamin Poulain <benjamin at webkit.org>  2011-05-27 10:33:16 PST ---
(From update of attachment 95182)
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.

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