Bug 88096 – [Qt][WK2] Refactor the tap gesture recognizer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andras Becsi]

[Qt][WK2] Refactor the tap gesture recognizer

[Andras Becsi]

Created attachment 145313 [details]
Patch

[Simon Hausmann]

Comment on attachment 145313 [details]
Patch

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();

[Andras Becsi]

Committed r119385: <http://trac.webkit.org/changeset/119385>

[Andras Becsi]

Comment on attachment 145313 [details]
Patch

Clearing flags from attachment.