[webkit-reviews] review denied: [Bug 83091] [Qt][WK2] Click, mouse and links rely on touch mocking : [Attachment 155290] proposed patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Andras Becsi
<abecsi at webkit.org>'s request for review:
Bug 83091: [Qt][WK2] Click, mouse and links rely on touch mocking
https://bugs.webkit.org/show_bug.cgi?id=83091

Attachment 155290: proposed patch
https://bugs.webkit.org/attachment.cgi?id=155290&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=155290&action=review


Looks good in general, a couple of small issues and questions :)

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:460
> +	   isMouseEvent = true;
>      case QEvent::TouchBegin:

I think between those two lines we usually add a "// Fall through" comment, to
indicate that the "fall through" is deliberate.

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:463
> +	   m_isMouseButtonPressed = true;

Shouldn't this be set in QEvent::MouseButtonPress?

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:476
> +	   isMouseEvent = true;
>      case QEvent::TouchUpdate:

Fall through comment.

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:483
> +	   isMouseEvent = true;
>      case QEvent::TouchEnd:

And here :)

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:484
> +	   m_isMouseButtonPressed = false;

Same question as above: Should this be in case QEvent::MouseButtonRelease?

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:515
> +	   currentTouchPoint.setId(mouseEvent->buttons());

Maybe worth a comment, it's easy to overlook this trick when glancing through
the code :)

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:517
> +	   currentTouchPoint.setRect(QRectF(mouseEvent->localPos(), QSizeF(40,
40)));

Hm, why 40, 40 instead of 1, 1?