[webkit-reviews] review granted: [Bug 88096] [Qt][WK2] Refactor the tap gesture recognizer : [Attachment 145313] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has granted Andras Becsi
<abecsi at webkit.org>'s request for review:
Bug 88096: [Qt][WK2] Refactor the tap gesture recognizer
https://bugs.webkit.org/show_bug.cgi?id=88096

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

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145313&action=review


r=me

> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp:96
> +	   // The singleTapTimeout was ignored earlier but the gesture has been
finished
> +	   // before the tapAndHoldTimeout fired, therefore this is a single
tap gesture.
> +	   m_eventHandler->handleSingleTapEvent(touchPoint);

I think it would help if the comment would directly say that this happens when
the finger is released (gesture finished) after the single tap timeout elapsed
(500ms) but _before_ the tapAndHold (1000ms), i.e. explain it using numbers
instead of states. Just a thought :)

> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.h:39
>  const int maxPanDistance = 10;
>  const int maxDoubleTapDistance = 120;
> -const int tapAndHoldTime = 800;
> -const int maxDoubleTapInterval = 400;
> -const int highlightDelay = 80;
> +const int tapAndHoldTime = 1000;
> +const int maxDoubleTapInterval = 500;
> +const int highlightDelay = 100;

Why are these global constants in a header file? I understand that due to them
being const they will only have internal linkage, but nevertheless they are not
shared with any other files and IMHO would fit just as well into the .cpp file.


I also wonder if these timeouts should be consistent across the platform where
tap gestures are recognized in different places, i.e. if they should be read
from some setting? (think platform plugin)

For example I can see that our tapAndHoldTime is 1 second while in
qquickmouseareap.cpp it's 800ms. That's subtle, but still different.

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:505
>	       if (!m_panGestureRecognizer.isRecognized())
> -		   m_tapGestureRecognizer.update(ev->type(),
touchPoints.first());
> +		   m_tapGestureRecognizer.finish(touchPoints.first());
>	       m_panGestureRecognizer.finish(touchPoints.first(),
eventTimestampMillis);

I think this code would be more readable if it was

    if (m_panGestureRecognizer.isRecognized())
	m_panGestureRecognizer.finish(...);
    else
	m_tapGestureRecognizer.finish();