[webkit-reviews] review granted: [Bug 64373] [Qt][WK2] Add a basic Pinch gesture recognizer for WebKit 2 : [Attachment 100528] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 64373: [Qt][WK2] Add a basic Pinch gesture recognizer for WebKit 2
https://bugs.webkit.org/show_bug.cgi?id=64373

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

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100528&action=review


r=me with some itches.

> Source/WebKit2/UIProcess/qt/QtPinchGestureRecognizer.cpp:68
> +    case QEvent::TouchBegin:
> +    case QEvent::TouchUpdate:

Would be neat with a comment here explaining that the ultimate plan is to only
initiate the gesture upon TouchBegin.

> Source/WebKit2/UIProcess/qt/QtPinchGestureRecognizer.h:42
> +const qreal pinchInitialTriggerDistanceThreshold = 5.;

Why not put this in the cpp file?

> Source/WebKit2/UIProcess/qt/TouchViewInterface.cpp:34
> +    , m_pinchStartScale(1.f)

Coding style, unnecessary .f suffix.

> Source/WebKit2/UIProcess/qt/TouchViewInterface.cpp:69
> +void TouchViewInterface::pinchGestureRequestScale(const QPointF&
pinchCenterInPageViewCoordinate, qreal totalScaleFactor)

The name of this function is a bit weird. setScaleFromPinchGesture() perhaps?

> Source/WebKit2/UIProcess/qt/TouchViewInterface.h:44
> +    void pinchGestureStarted();
> +    void pinchGestureRequestScale(const QPointF&, qreal);
> +    void pinchGestureEnded();

I would have used did* style naming for these.