Bug 83044

Summary: [Qt][WK2] Refactor the gesture recognizers
Product: WebKit Reporter: Andras Becsi <abecsi>
Component: New BugsAssignee: Andras Becsi <abecsi>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann, jturcotte, kenneth, menard, webkit.review.bot, zoltan
Priority: P3 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: Unspecified   
Bug Depends on: 83033    
Bug Blocks: 76773    
Attachments:
Description Flags
proposed patch
kenneth: review+
patch for landing none

Description Andras Becsi 2012-04-03 10:15:52 PDT
The transition between different gestures is not as smooth as it should be. The gesture recognizers need some refactoring.
Comment 1 Andras Becsi 2012-04-13 08:42:31 PDT
Created attachment 137091 [details]
proposed patch
Comment 2 Simon Hausmann 2012-04-16 11:32:48 PDT
Comment on attachment 137091 [details]
proposed patch

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

I like this a lot. Much red, much clearer code, less indentation :). A few comments below. I'm sure Kenneth will want to have a look, too :)

> Source/WebKit2/UIProcess/qt/QtPinchGestureRecognizer.cpp:58
> +    case GestureRecognitionStarted:
> +    case GestureRecognized:
> +        if (m_state == GestureRecognitionStarted) {

Wouldn't it be easier to read to use a "fall-through"?

    switch (m_state) {
    case GestureRecognitionStarted:
        const qreal pinchDistance = ...;
        ...
        // fall through
    case GestureRecognized:
       const qreal totalScaleFactor =

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:477
> +    QList<QTouchEvent::TouchPoint> activeTouchPoints;

It might be worth it to reserve() on the list (with touchPoints.size()).

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:479
> +    for (int i = 0; i < touchPoints.size(); ++i) {

According to the coding style I don't think you need curly braces here.

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:484
> +    if (!activeTouchPoints.size()) {

Readability nitpick-preference: Since you're also comparing against 1 and 2, I think comparing against 0 is clearer than the ! syntax. (IMHO)
Comment 3 Kenneth Rohde Christiansen 2012-04-16 15:00:28 PDT
(In reply to comment #2)
> (From update of attachment 137091 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137091&action=review
> 
> I like this a lot. Much red, much clearer code, less indentation :). A few comments below. I'm sure Kenneth will want to have a look, too :)

It should be :-) we discussed a lot how to improve it. I will have a proper look at it, but I guess that will first be after I arrive in the US. I hope that is OK.
Comment 4 Kenneth Rohde Christiansen 2012-04-16 15:03:44 PDT
Comment on attachment 137091 [details]
proposed patch

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

>> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:479
>> +    for (int i = 0; i < touchPoints.size(); ++i) {
> 
> According to the coding style I don't think you need curly braces here.

You do indeed, the content of the for loop is more than one actual (not logical) line. Same counts if we add comments inside

>> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:484
>> +    if (!activeTouchPoints.size()) {
> 
> Readability nitpick-preference: Since you're also comparing against 1 and 2, I think comparing against 0 is clearer than the ! syntax. (IMHO)

The code style says to never compare against 0. So this is following the webkit style
Comment 5 Kenneth Rohde Christiansen 2012-04-16 16:11:52 PDT
Comment on attachment 137091 [details]
proposed patch

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

I notice that the pinch recognizer is never cancelled, only finished. There is also no code handling more than two fingers.

> Source/WebKit2/ChangeLog:10
> +        on the basis of how many active touch points the current touch event has.
> +        Active touch points are pressed, moved or stationary and the number of these

I would like a newline between these lines

> Source/WebKit2/UIProcess/qt/QtPanGestureRecognizer.cpp:90
> +    reset();

im wondering how much reset is doing now, and whether we can get rid of it (it is virtual right now I believe)

> Source/WebKit2/UIProcess/qt/QtPanGestureRecognizer.h:39
>  class QtPanGestureRecognizer : public QtGestureRecognizer {

That inheriting might not make much sense anymore

> Source/WebKit2/UIProcess/qt/QtPinchGestureRecognizer.cpp:50
> +    const qreal currentSpanDistance = QLineF(point1.screenPos(), point2.screenPos()).length();

FingerDistance?

> Source/WebKit2/UIProcess/qt/QtPinchGestureRecognizer.cpp:66
> +            // touch points in order to avoid the jump caused by skipping all the

by the events who were skipped between the recognition start and the actual recognition.

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:485
> +        if (touchPoints.size() == 1)

Maybe comment would make this easier to understand, like: // No active touch points, one finger lifted.

Or int fingersLifted = touchPoints.size()

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:490
> +        m_pinchGestureRecognizer.finish();

I guess this should be cancel().
Comment 6 Andras Becsi 2012-04-17 09:40:44 PDT
Created attachment 137548 [details]
patch for landing
Comment 7 Andras Becsi 2012-04-17 09:41:43 PDT
Committed r114389: <http://trac.webkit.org/changeset/114389>