[webkit-reviews] review canceled: [Bug 71841] [Qt][WK2] Add Tap Gesture recognition to UIProcess : [Attachment 114137] patch for feedback.

[bugzilla-daemon at webkit.org]

Zeno Albisser <zeno at webkit.org> has canceled Zeno Albisser <zeno at webkit.org>'s
request for review:
Bug 71841: [Qt][WK2] Add Tap Gesture recognition to UIProcess
https://bugs.webkit.org/show_bug.cgi?id=71841

Attachment 114137: patch for feedback.
https://bugs.webkit.org/attachment.cgi?id=114137&action=review

------- Additional Comments from Zeno Albisser <zeno at webkit.org>
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 <zeno at webkit.org>
> 
> 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.