[webkit-reviews] review denied: [Bug 32432] [Qt] Add support for touch events in QWebView and QGraphicsWebView : [Attachment 44866] Patch for adding support for touch events in QWebView and QGraphicsWebView.

[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 32432: [Qt] Add support for touch events in QWebView and QGraphicsWebView
https://bugs.webkit.org/show_bug.cgi?id=32432

Attachment 44866: Patch for adding support for touch events in QWebView and
QGraphicsWebView.
https://bugs.webkit.org/attachment.cgi?id=44866&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
The patch looks good to me! Just a few minor style nitpicks that need to be
fixed before we can land this:

> +
> +#if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)
> +    if (d->page) {
> +	   if (event->type() == QEvent::TouchBegin 
> +	       || event->type() == QEvent::TouchEnd 
> +	       || event->type() == QEvent::TouchUpdate) {
> +		   d->page->event(event);
> +	   }

The curly braces are left out when the body is only a one-liner.

I would recommend combining the two if statements:

if (d->page && event->type() == ...
    && ... )
    d->page->event(event);

Also, with the above code we always call QWidget::event() with the touch event,
which will
convert the first touch to a mouse event (according to the docs).

I think perhaps the code should be like:

if (...) {
    d->page->event(event);
    if (event->isAccepted())
	return true;
}

...
else we call QWidget::event.

The same for QGraphicsWebView and QWebView of course.