Bug 71841 – [Qt][WK2] Add Tap Gesture recognition to UIProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Zeno Albisser]

SSIA

[Zeno Albisser]

Created attachment 114137 [details]
patch for feedback.

[Kenneth Rohde Christiansen]

Comment on attachment 114137 [details]
patch for feedback.

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

> Source/WebKit2/ChangeLog:8
> +        Add a Tap gesture recognizer that delivers GestureSingleTap
> +        and GestureTapAndHold events through the WebPageProxy.
> +        Add a GestureTapAndHold to WebEvent.

As this is based on code from Benjamin, I would add that. Give credit where due.
Btw you seem to support double tap as well!

> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp:2
> + * Copyright (C) 2011 Zeno Albisser <[e-mail]>

We normally add Nokia copyright, though I don't know our official stand on this

> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp:37
> +    , m_tapState(NoPointing)

Weird name

> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp:61
> +        if (m_tapDoubleClickTimer.isActive()) {

We really need to make sure that hte time constants match our platform. Ie. I think the harmattan platform ignores too fast taps as well. We might need to support that.

> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp:68
> +                m_tapState = DoubleTapStarted;

It is kind of already recognized... why not call it DoubleTapRecognized ? or better yet, DoubleTapCandidate

> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp:104
> +            {
> +                ASSERT(!m_tapDoubleClickTimer.isActive());
> +                m_tapState = NoPointing;
> +
> +                const QTouchEvent::TouchPoint& touchPoint = event->touchPoints().first();
> +                QPointF startPosition = touchPoint.startScreenPos();
> +                QPointF endPosition = touchPoint.screenPos();
> +                if (QLineF(endPosition, startPosition).length() < maxDoubleTapDistance && m_webPageProxy)
> +                    m_webPageProxy->findZoomableAreaForPoint(touchPoint.pos().toPoint());
> +                break;
> +            }

wrong indention I believe (chck the style guide)

I am not such a fan of calling code (findZoomableAreaForPoint) directly from the recognizer... people will not find that easily and it is not well separated... better call a method on the interaction engine such as "interactionEngine->doubleTapGesture()"

> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.h:42
> +const qreal initialTriggerDistanceThreshold = 5;
> +const qreal maxDoubleTapDistance = 120;
> +const int tapAndHoldTime = 800;
> +const int doubleClickInterval = 400;

These should probably consider DPI. Maybe add a FIXME

> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.h:60
> +    QBasicTimer m_tapDoubleClickTimer;

doubleTapTimer?

> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.h:65
> +        NoPointing,

NoTap would make more sense

> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.h:74
> +#endif /* QtPanGestureRecognizer_h */

This is not correct

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:-830
>      if (wasEventHandled || event.type() == WebEvent::TouchCancel) {
>          m_panGestureRecognizer.reset();
>          m_pinchGestureRecognizer.reset();
> -        return;

Don't you want to reset the tap one here? Or add a comment why not?

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:837
> +        const QTouchEvent* ev = event.nativeEvent();

Why not put this outside, you are doing the same lower down

[Kenneth Rohde Christiansen]

Comment on attachment 114137 [details]
patch for feedback.

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

> Source/WebKit2/Shared/WebEvent.h:67
>          GestureScrollBegin,
>          GestureScrollEnd,
>          GestureSingleTap,
> +        GestureTapAndHold,

Why is there no GestureDoubleTap?

[Zeno Albisser]

Comment on attachment 114137 [details]
patch for feedback.

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

i'll post an update as soon as i figured out a good solution for sending the findZoomableAreaForPoint() call through the QtViewportInteractionEngine.

>> Source/WebKit2/ChangeLog:8
>> +        Add a GestureTapAndHold to WebEvent.
> 
> As this is based on code from Benjamin, I would add that. Give credit where due.
> Btw you seem to support double tap as well!

sure.

>> Source/WebKit2/Shared/WebEvent.h:67
>> +        GestureTapAndHold,
> 
> Why is there no GestureDoubleTap?

the double tap gesture is not sent through the webPageProxy as an event. It just triggers findZoomableAreaForPoint(..).
As far as i understand, the async response should then be picked up by QtWebPageProxy::didFindZoomableArea(..).
But i think i need to look into that again in more detail.

>> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp:2
>> + * Copyright (C) 2011 Zeno Albisser <[e-mail]>
> 
> We normally add Nokia copyright, though I don't know our official stand on this

I did it the same way it is for QtPanGestureRecognizer.h/.cpp. But you are right, i can find all sorts of examples.
I will just change it to Nokia, i guess there can nothing be wrong with that.

>> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp:37
>> +    , m_tapState(NoPointing)
> 
> Weird name

i guess you mean the "NoPointing" thing. I will change that to.... ... ... something different.
Do you think "NoTap" is better?

>> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp:61
>> +        if (m_tapDoubleClickTimer.isActive()) {
> 
> We really need to make sure that hte time constants match our platform. Ie. I think the harmattan platform ignores too fast taps as well. We might need to support that.

ahm... ... yes.

>> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp:68
>> +                m_tapState = DoubleTapStarted;
> 
> It is kind of already recognized... why not call it DoubleTapRecognized ? or better yet, DoubleTapCandidate

I see your point there. But it is not quite correct to say, that the gesture is already recognized. The gesture will be recognized (completely) once the TouchEnd event comes in.
Theoretically it is still possible that the gesture will be canceled before that. - I think we should leave that one.

>> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp:104
>> +            }
> 
> wrong indention I believe (chck the style guide)
> 
> I am not such a fan of calling code (findZoomableAreaForPoint) directly from the recognizer... people will not find that easily and it is not well separated... better call a method on the interaction engine such as "interactionEngine->doubleTapGesture()"

indentation: i was surprised as well. But that is what check-webkit-style wanted me to do. And the style guide doesn't say anything about scopes in switch case constructs.
I could get rid of the scope of course. it is not absolutely necessary. The reason why i left it, is that i am creating objects/variables inside that case-break construct and it seemed to be the safest way like that. Shall i change that, or leave it?

Yeah, right. That would possibly fit in there. I'll see if i can change that.

>> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.h:60
>> +    QBasicTimer m_tapDoubleClickTimer;
> 
> doubleTapTimer?

done.

>> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.h:65
>> +        NoPointing,
> 
> NoTap would make more sense

oh... here i have my answer to one of the previous questions already. :-)

>> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.h:74
>> +#endif /* QtPanGestureRecognizer_h */
> 
> This is not correct

right.

>> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:-830
>> -        return;
> 
> Don't you want to reset the tap one here? Or add a comment why not?

Yes, we should. - done.

>> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:837
>> +        const QTouchEvent* ev = event.nativeEvent();
> 
> Why not put this outside, you are doing the same lower down

yeah i can optimize that i think.

[Kenneth Rohde Christiansen]

>> It is kind of already recognized... why not call it DoubleTapRecognized ? or better yet, DoubleTapCandidate

>I see your point there. But it is not quite correct to say, that the gesture is already recognized. The gesture will be recognized (completely) once the TouchEnd event comes in.
>Theoretically it is still possible that the gesture will be canceled before that. - I think we should leave that one.

That is why DoubleTapCandidate is a good word :-)

[Zeno Albisser]

Comment on attachment 114137 [details]
patch for feedback.

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

>>> Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp:104
>>> +            }
>> 
>> wrong indention I believe (chck the style guide)
>> 
>> I am not such a fan of calling code (findZoomableAreaForPoint) directly from the recognizer... people will not find that easily and it is not well separated... better call a method on the interaction engine such as "interactionEngine->doubleTapGesture()"
> 
> indentation: i was surprised as well. But that is what check-webkit-style wanted me to do. And the style guide doesn't say anything about scopes in switch case constructs.
> I could get rid of the scope of course. it is not absolutely necessary. The reason why i left it, is that i am creating objects/variables inside that case-break construct and it seemed to be the safest way like that. Shall i change that, or leave it?
> 
> Yeah, right. That would possibly fit in there. I'll see if i can change that.

After looking into the ViewportInteractionEngine again i consider it not the right place for wiring findZoomableAreaForPoint(..). The Interaction engine does not have the required information available. Further we are directly talking to the WebProcess in that case, updating the viewport should be a reaction to the response of the WebProcess.

[Zeno Albisser]

(In reply to comment #5)
> >> It is kind of already recognized... why not call it DoubleTapRecognized ? or better yet, DoubleTapCandidate
> 
> >I see your point there. But it is not quite correct to say, that the gesture is already recognized. The gesture will be recognized (completely) once the TouchEnd event comes in.
> >Theoretically it is still possible that the gesture will be canceled before that. - I think we should leave that one.
> 
> That is why DoubleTapCandidate is a good word :-)

i can live with that, i think.

[Zeno Albisser]

Created attachment 114295 [details]
patch for review.

[Zeno Albisser]

Comment on attachment 114295 [details]
patch for review.

Clearing flags on attachment: 114295

Committed r99831: <http://trac.webkit.org/changeset/99831>

[Zeno Albisser]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

This broke the Lion builds. The mac bot also indicates that this broke Mac OS.
Please fix it.

[Zeno Albisser]

fixed in r99837