Bug 83044

Created attachment 137091 [details] proposed patch

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)

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

Created attachment 137548 [details] patch for landing

Committed r114389: <http://trac.webkit.org/changeset/114389>