[webkit-reviews] review granted: [Bug 83044] [Qt][WK2] Refactor the gesture recognizers : [Attachment 137091] proposed patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Andras Becsi
<abecsi at webkit.org>'s request for review:
Bug 83044: [Qt][WK2] Refactor the gesture recognizers
https://bugs.webkit.org/show_bug.cgi?id=83044

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=137091&action=review


I notice that the pinch recognizer is never cancelled, only finished. There is
also no code handling more than two fingers.

> Source/WebKit2/ChangeLog:10
> +	   on the basis of how many active touch points the current touch event
has.
> +	   Active touch points are pressed, moved or stationary and the number
of these

I would like a newline between these lines

> Source/WebKit2/UIProcess/qt/QtPanGestureRecognizer.cpp:90
> +    reset();

im wondering how much reset is doing now, and whether we can get rid of it (it
is virtual right now I believe)

> Source/WebKit2/UIProcess/qt/QtPanGestureRecognizer.h:39
>  class QtPanGestureRecognizer : public QtGestureRecognizer {

That inheriting might not make much sense anymore

> Source/WebKit2/UIProcess/qt/QtPinchGestureRecognizer.cpp:50
> +    const qreal currentSpanDistance = QLineF(point1.screenPos(),
point2.screenPos()).length();

FingerDistance?

> Source/WebKit2/UIProcess/qt/QtPinchGestureRecognizer.cpp:66
> +	       // touch points in order to avoid the jump caused by skipping
all the

by the events who were skipped between the recognition start and the actual
recognition.

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:485
> +	   if (touchPoints.size() == 1)

Maybe comment would make this easier to understand, like: // No active touch
points, one finger lifted.

Or int fingersLifted = touchPoints.size()

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:490
> +	   m_pinchGestureRecognizer.finish();

I guess this should be cancel().