[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


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #92971|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #11 from Benjamin Poulain <benjamin at webkit.org>  2011-05-18 10:04:30 PST ---
(From update of attachment 92971)
A synthetic click usually include the mouse move, which will ensure the mouseover/mouseout.

A quick review, mostly minor stuff.
My main concern is the lack of mouse move for synthetic click, that will break some websites.


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

> Source/WebCore/platform/PlatformGestureRecognizer.h:62
> +    // Clears the GestureRecognizer to its initial state. It's necessary to call this before each
> +    // 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.

> Source/WebCore/platform/PlatformWheelEvent.h:109
> +        PlatformWheelEvent(IntPoint position,

Please also initialize the MAC attribute to there default values, just for completeness.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:85
> +    int manhattanDistance = abs(point.pos().x() - m_firstTouchPosition.x()) +
> +        abs(point.pos().y() - m_firstTouchPosition.y());

Just style: if you really want to split this line, please align the two abs().

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:96
> +void GestureRecognizerChromium::dispatchSyntheticClick(const PlatformTouchPoint& point)
> +{
> +    PlatformMouseEvent fakeMouseDown(point.pos(), point.screenPos(), LeftButton, MouseEventPressed, 1, false, false, false, false, m_lastTouchTime);
> +    PlatformMouseEvent fakeMouseUp(point.pos(), point.screenPos(), LeftButton, MouseEventReleased, 1, false, false, false, false, m_lastTouchTime);
> +
> +    m_eventHandler->handleMousePressEvent(fakeMouseDown);
> +    m_eventHandler->handleMouseReleaseEvent(fakeMouseUp);
> +}

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).

> 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?

> 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.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:210
> +    gestureRecognizer->init();

Any reason not to initialize that in the constructor?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:211
> +    return PassOwnPtr<PlatformGestureRecognizer>(gestureRecognizer);

You should use adoptPtr here.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:219
> +PlatformGestureRecognizer::PlatformGestureRecognizer()
> +{
> +}
> +PlatformGestureRecognizer::~PlatformGestureRecognizer()
> +{
> +}

Wouldn't it be cleaner to include the original cpp file?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.h:42
> +class GestureRecognizerChromium;

You probably don't need this.

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