[webkit-reviews] review denied: [Bug 54417] [chromium] Add a basic gesture recognizer to the Chromium platform : [Attachment 92971] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Robert Kroeger
<rjkroege at chromium.org>'s request for review:
Bug 54417: [chromium] Add a basic gesture recognizer to the Chromium platform
https://bugs.webkit.org/show_bug.cgi?id=54417

Attachment 92971: Patch
https://bugs.webkit.org/attachment.cgi?id=92971&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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.