[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


Robert Kroeger <rjkroege at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #95182|1                           |0
        is obsolete|                            |




--- Comment #25 from Robert Kroeger <rjkroege at chromium.org>  2011-06-01 15:17:00 PST ---
(From update of attachment 95182)
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.

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