[Qt] DRT EventSender support to graphics mouse events activation

[Diego Gonzalez]

Make Qt DRT send the correct QGraphicsSceneEvent's when it is
in QGraphicsWebView mode.

[Diego Gonzalez]

Created attachment 58151 [details]
Patch

[Antonio Gomes]

lgtm.

I'd rename hasGraphicsEvents , since it does not sound good to me. Maybe isGraphicsBasedView() ? or something ...

or even better, just remove it, since you do not need this method. Inside EventSender class you can know if it is GraphicsBased or not, by checking page->view() - as you did in EventSender::sendEvent

[Diego Gonzalez]

(In reply to comment #2)
> lgtm.
> 
> I'd rename hasGraphicsEvents , since it does not sound good to me. Maybe isGraphicsBasedView() ? or something ...
> 
> or even better, just remove it, since you do not need this method. Inside EventSender class you can know if it is GraphicsBased or not, by checking page->view() - as you did in EventSender::sendEvent

Make sense Antonio.

[Diego Gonzalez]

Created attachment 58189 [details]
Patch

[Antonio Gomes]

comments:

(In reply to comment #4)
> Created an attachment (id=58189) [details]
> Patch

+        event->setModifiers(Qt::NoModifier);
+        sendOrQueueEvent(event);
+    } else {
+        QMouseEvent* event = new QMouseEvent(QEvent::MouseButtonRelease, m_mousePos, m_mousePos, mouseButton, m_mouseButtons, Qt::NoModifier);
+        sendOrQueueEvent(event);

I'd put "endOrQueueEvent(event);" out of the if-else block, since it will be executed in both cases. It happens twice in the code.

if (WebCore::WebViewGraphicsBased* view ...

do you need the WebCore namespace?

[Diego Gonzalez]

(In reply to comment #5)
> comments:
> 
> (In reply to comment #4)
> > Created an attachment (id=58189) [details] [details]
> > Patch
> 
> +        event->setModifiers(Qt::NoModifier);
> +        sendOrQueueEvent(event);
> +    } else {
> +        QMouseEvent* event = new QMouseEvent(QEvent::MouseButtonRelease, m_mousePos, m_mousePos, mouseButton, m_mouseButtons, Qt::NoModifier);
> +        sendOrQueueEvent(event);
> 
> I'd put "endOrQueueEvent(event);" out of the if-else block, since it will be executed in both cases. It happens twice in the code.
> 

The events types are different in this case, so more casts will
be necessary.

> if (WebCore::WebViewGraphicsBased* view ...
> 
> do you need the WebCore namespace?

Unfortunately the namespace is necessary :(. A little refactory i, DumpRenderTreeQt will be necessary to remove it,

[Diego Gonzalez]

The results with this patch running in graphics view mode:

13767 test cases (99%) succeeded
19 test cases (<1%) had incorrect layout
49 test cases (<1%) were new
1 test case (<1%) crashed
5 test cases (<1%) had stderr output

Before this patch:

13502 test cases (97%) succeeded
270 test cases (1%) had incorrect layout
38 test cases (<1%) were new
2 test cases (<1%) crashed
162 test cases (1%) had stderr output

Results with normal mode (QWebView):

13778 test cases (99%) succeeded
9 test cases (<1%) had incorrect layout
49 test cases (<1%) were new
5 test cases (<1%) had stderr output

[Kenneth Rohde Christiansen]

Comment on attachment 58189 [details]
Patch


> +    if (qobject_cast<WebCore::WebViewGraphicsBased*>(m_page->view()))
> +        m_graphicsBasedView = true;
> +    else
> +        m_graphicsBasedView = false;

Make a method for that

bool isGraphicsBased() { return qobject_cast<WebCore::WebViewGraphicsBased*>(m_page->view()); }

[Diego Gonzalez]

Created attachment 58240 [details]
Patch

Make Kenneth's suggestion

[Kenneth Rohde Christiansen]

Comment on attachment 58240 [details]
Patch

Wouldn't it be possible to make methods like QGraphicsSceneMouseEvent* createGraphicsSceneMouseEvent(... ) etc.

[Diego Gonzalez]

(In reply to comment #10)
> (From update of attachment 58240 [details])
> Wouldn't it be possible to make methods like QGraphicsSceneMouseEvent* createGraphicsSceneMouseEvent(... ) etc.

You mean a method similar to QMouseEvent constructor? Like this:
QGraphicsSceneMouseEvent* createGraphicsSceneMouseEvent(QEvent::Type type, const QPoint& pos, const QPoint& screenPos, Qt::MouseButton button, Qt::MouseButtons buttons, Qt::KeyboardModifiers modifiers);

[Diego Gonzalez]

Created attachment 58263 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 58263 [details]
Patch


> +void EventSender::sendEvent(QObject* receiver, QEvent* event, bool graphics)

Couldn't it verify this "graphics" inside the method? This is never going to change through runs anyway.

> +{
> +    if (!graphics)
> +        QApplication::sendEvent(receiver, event);
> +    else {
> +        if (WebCore::WebViewGraphicsBased* view = qobject_cast<WebCore::WebViewGraphicsBased*>(receiver))
> +            view->scene()->sendEvent(view->graphicsView(), event);
> +    }
> +}

[Diego Gonzalez]

Created attachment 58268 [details]
Patch

[Diego Gonzalez]

Created attachment 58288 [details]
Patch

[Diego Gonzalez]

Created attachment 58292 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 58292 [details]
Patch

Clearing flags on attachment: 58292

Committed r60959: <http://trac.webkit.org/changeset/60959>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.