Bug 88096 - [Qt][WK2] Refactor the tap gesture recognizer
Summary: [Qt][WK2] Refactor the tap gesture recognizer
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andras Becsi
Depends on:
Reported: 2012-06-01 08:09 PDT by Andras Becsi
Modified: 2012-06-04 04:43 PDT (History)
7 users (show)

See Also:

Patch (14.58 KB, patch)
2012-06-01 08:11 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andras Becsi 2012-06-01 08:09:13 PDT
[Qt][WK2] Refactor the tap gesture recognizer
Comment 1 Andras Becsi 2012-06-01 08:11:30 PDT
Created attachment 145313 [details]
Comment 2 Simon Hausmann 2012-06-01 23:27:14 PDT
Comment on attachment 145313 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=145313&action=review


> 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())
Comment 3 Andras Becsi 2012-06-04 04:31:28 PDT
Committed r119385: <http://trac.webkit.org/changeset/119385>
Comment 4 Andras Becsi 2012-06-04 04:43:02 PDT
Comment on attachment 145313 [details]

Clearing flags from attachment.