[webkit-reviews] review denied: [Bug 32434] [Qt] Add support for mocking touch events with Q(GV)Launcher : [Attachment 44954] Adds multitouch mocking to QtLauncher

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Kim Grönholm
<kim.gronholm at nomovok.com>'s request for review:
Bug 32434: [Qt] Add support for mocking touch events with Q(GV)Launcher
https://bugs.webkit.org/show_bug.cgi?id=32434

Attachment 44954: Adds multitouch mocking to QtLauncher
https://bugs.webkit.org/attachment.cgi?id=44954&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>

> +#if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)
> +#include <qevent.h>
> +#endif

This #if isn't necessary, qevent.h has been there since Qt 1.0 :-)
  
> +    bool eventFilter(QObject* obj, QEvent* event)
> +    {
> +	   if (!touchMocking || obj != view)
> +	       return QObject::eventFilter(obj, event);
> +
> +	   if (event->type() == QEvent::MouseButtonPress
> +	       || event->type() == QEvent::MouseButtonRelease
> +	       || event->type() == QEvent::MouseButtonDblClick
> +	       || event->type() == QEvent::MouseMove) {
> +
> +	       QMouseEvent* ev = static_cast<QMouseEvent *>(event);

Coding style nitpick: No space before the * :)

> +	       if (ev->type() == QEvent::MouseMove
> +		   && !(ev->buttons() & Qt::LeftButton))
> +		   return false;
> +
> +	       QTouchEvent::TouchPoint touchPoint;
> +	       touchPoint.setState(Qt::TouchPointMoved);
> +	       if ((ev->type() == QEvent::MouseButtonPress
> +		    || ev->type() == QEvent::MouseButtonDblClick))
> +		   touchPoint.setState(Qt::TouchPointPressed);
> +	       else if (ev->type() == QEvent::MouseButtonRelease)
> +		   touchPoint.setState(Qt::TouchPointReleased);
> +
> +	       touchPoint.setId(0);
> +	       touchPoint.setScreenPos(ev->globalPos());
> +	       touchPoint.setPos(ev->pos());
> +	       touchPoint.setPressure(1);
> +
> +	       // If the point already exists, update it. Otherwise create it.
> +	       if (touchPoints.size()>0 && !touchPoints[0].id()) 

Coding style: There should be a space before and after the > character

> +		   touchPoints[0] = touchPoint;
> +	       else if (touchPoints.size()>1 && !touchPoints[1].id()) 

Ditto.

> +		   touchPoints[1] = touchPoint;
> +	       else 
> +		   touchPoints.append(touchPoint);
> +
> +	       sendTouchEvent();
> +	   } else if (event->type() == QEvent::KeyPress
> +	       && ((QKeyEvent*)event)->key() == Qt::Key_F
> +	       && ((QKeyEvent*)event)->modifiers() == Qt::ControlModifier) {

Please use a static_cast.

> +	       // If the keyboard point is already pressed, release it.
> +	       // Otherwise create it and append to touchPoints.
> +	       if (touchPoints.size()>0 && touchPoints[0].id() == 1) {

Coding style: There should be a space before and after the > character

> +		   touchPoints[0].setState(Qt::TouchPointReleased);
> +		   sendTouchEvent();
> +	       } else if (touchPoints.size()>1 && touchPoints[1].id() == 1) {

Ditto.

> +		   touchPoints[1].setState(Qt::TouchPointReleased);
> +		   sendTouchEvent();
> +	       } else {
> +		   QTouchEvent::TouchPoint touchPoint;
> +		   touchPoint.setState(Qt::TouchPointPressed);
> +		   touchPoint.setId(1);
> +		   touchPoint.setScreenPos(QCursor::pos());
> +		   touchPoint.setPos(view->mapFromGlobal(QCursor::pos()));

I suggest to remember the last mouse point. Just a thought :)

> +		   touchPoint.setPressure(1);		 
> +		   touchPoints.append(touchPoint);
> +		   sendTouchEvent();
> +
> +		   // After sending the event, change the touchpoint state to
stationary
> +		   touchPoints.last().setState(Qt::TouchPointStationary);


Shouldn't we disable mocking by default?