[Webkit-unassigned] [Bug 49345] [Chromium] Add a basic GestureManager to Chromium

[bugzilla-daemon at webkit.org]

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





--- Comment #16 from Benjamin Poulain <benjamin.poulain at nokia.com>  2010-12-11 03:55:44 PST ---
(From update of attachment 76281)
Comments from a quick look at the patch:


View in context: https://bugs.webkit.org/attachment.cgi?id=76281&action=review

> LayoutTests/fast/events/touch/touch-gesture-click.html:21
> +var EXPECTED_CLICK_EVENTS_TOTAL = 3;

I would rename this to expected _mouse_ events. Because only one click event is expected so the name can be confusing.

> LayoutTests/fast/events/touch/touch-gesture-click.html:25
> +var haveTouchEvents = false;

This does not seem to be used anywhere.

> LayoutTests/fast/events/touch/touch-gesture-click.html:30
> +        lastEvent = event;

You could get rid of lastEvent entirely and use event directly here.

> LayoutTests/fast/events/touch/touch-gesture-scroll.html:66
> +var movingdiv;

This does not need to be global, you are getting the element directly in the functions.

> LayoutTests/fast/events/touch/touch-gesture-scroll.html:159
> +        if (eventSender.clearTouchPoints) {
> +            firstTouchDragSequence();
> +        } else {
> +            exitIfNecessary();
> +        }

No brackets here according to the coding convention? (no idea what our conventions are for JavaScript :))

> WebCore/page/EventHandler.cpp:2989
> +    m_gestureManager->processTouchEventForGesture(event, this, defaultPrevented);

I guess this should be something like:
if (!defaultPrevented)
    defaultPrevented = m_gestureManager->processTouchEventForGesture(event, this);

You pass defaultPrevented in the gesture manager but I did not see used anywhere. If the page is handling the touch event, I would not add a WebKit gesture on top (e.g.: you don't want to scroll the frame when panning the touch version of Google maps).

You also need to change the value of defaultPrevented if processTouchEventForGesture does something with the event. If not, platforms usually pass the event to the parent widget.

> WebCore/page/GestureManager.cpp:62
> +    , m_eventHandler(0)

I think this should be passed as an argument of the constructor. There is a relation 1<->1 between the EventHandler and the GestureManager. I don't see the point of passing it in GestureManager::processTouchEventForGesture().

> WebCore/page/GestureManager.cpp:217
> +// To create a richer gesture recognizer, it is sufficient to modify this
> +// factory to add additional edges. (Though additional edges may require
> +// additional state tracking.)
> +GestureManager* createGestureManager()
> +{
> +    GestureManager* gm = new GestureManager();
> +    gm->init();
> +    return gm;
> +}

I am not sure why you need this instead of calling init from the constructor.
If subclassing the GestureManager is the way to extend it, adding additional edges could be done in the constructor of the subclass.

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