[webkit-reviews] review denied: [Bug 32484] [Qt] Add support for scale and transformation properties to TouchEvent : [Attachment 92986] patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Igor Trindade Oliveira
<itrindade.oliveira at gmail.com>'s request for review:
Bug 32484: [Qt] Add support for scale and transformation properties to
TouchEvent
https://bugs.webkit.org/show_bug.cgi?id=32484

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92986&action=review

> LayoutTests/ChangeLog:15
> +	   * fast/events/touch/touch-scale-rotation-expected.txt: Added.
> +	   * fast/events/touch/touch-scale-rotation.html: Added.

This only tests the forward/valid case. You should also test the values for the
cases:
-more than 2 fingers
-only one finger
-boundaries limits (like two very close fingers)

> Source/WebCore/platform/PlatformTouchEvent.h:67
> +	   , m_scale(1)
> +	   , m_rotationAngle(0)

You should use explicit floats for the literals, otherwise you might get a
warning for the conversion int->float.

> Source/WebCore/platform/PlatformTouchEvent.h:102
> +    float m_scale;
> +    float m_rotationAngle;

Don't you need to also modify the WebKit 2 parts to serialize those?

> Source/WebCore/platform/qt/PlatformTouchEventQt.cpp:33
> +static qreal handleRotationTouchEvent(const QList<QTouchEvent::TouchPoint>&
points)

I don't like handleRotationTouchEvent() and handleScaleTouchEvent() because
they rely on startPos(). This only works if the framework above only accept one
gesture at the time:
-pan
-lift your finger
-pinch
-lift your fingers
-etc.

In reality, most devs want to support the gesture following eachothers:
-pan
-keep fingers on screen
-pinch
-lift just one finger
-continue panning.

So the computation of handleRotationTouchEvent() and handleScaleTouchEvent()
need to be done from the last gesture started.
Gesture recognizer generally also have threshold, which needs to be taken into
account.

> Source/WebCore/platform/qt/PlatformTouchEventQt.cpp:57
> +	       / QLineF(firstPoint.startPos(), lastPoint.startPos()).length();

Splitting this line like this is a sign you should use temporary variables to
make this operation clearer :)