Bug 71310 – [Qt][WK2] Add support for touch event testing to WebKitTestRunner

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Hausmann]

[Qt][WK2] Add support for touch event testing to WebKitTestRunner

[Simon Hausmann]

Created attachment 113205 [details]
Patch work in progress

[Simon Hausmann]

Created attachment 113290 [details]
[Qt][WK2] Add support for touch event testing to WebKitTestRunner

[WebKit Review Bot]

Attachment 113290 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Tools/WebKitTestRunner/EventSenderProxy.h:55:  The parameter name "modifier" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/WebKitTestRunner/EventSenderProxy.h:71:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Created attachment 113292 [details]
[Qt][WK2] Add support for touch event testing to WebKitTestRunner

[Kenneth Rohde Christiansen]

View in context: https://bugs.webkit.org/attachment.cgi?id=113290&action=review

> Source/WebKit2/ChangeLog:15
> +        (QtDesktopWebPageProxy::handleTouchEvent): Forward touch events to WebPagePRoxy.

Spelling issue

> Source/WebKit2/UIProcess/WebPageProxy.cpp:963
> +        if (m_shouldSendEventsSynchronously) {

Make it more obvious that this is for testing only?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:512
> +    void touchEventSyncForTesting(const WebTouchEvent&, bool&);

it is not obvious what the bool means here. I would add 'handled'

> Tools/WebKitTestRunner/EventSenderProxy.h:54
> +    void updateTouchPoint(int index, int x, int y);

isn;t it more an id than an index? At least Lars told me that iOS uses ids and not indices

> Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:325
> +    if (index < 0 || index >= m_touchPoints.count())

Shouldn't this be an assert?

> Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:363
> +    for (int i = 0; i < m_touchPoints.count(); ++i)
> +        if (m_touchPoints[i].state() != Qt::TouchPointReleased) {
> +            sendTouchEvent(QEvent::TouchUpdate);
> +            return;
> +        }

Bad coding style... add braces

[Simon Hausmann]

(In reply to comment #5)
> View in context: https://bugs.webkit.org/attachment.cgi?id=113290&action=review
> 
> > Source/WebKit2/ChangeLog:15
> > +        (QtDesktopWebPageProxy::handleTouchEvent): Forward touch events to WebPagePRoxy.
> 
> Spelling issue

Oops. fixing :)

> > Source/WebKit2/UIProcess/WebPageProxy.cpp:963
> > +        if (m_shouldSendEventsSynchronously) {
> 
> Make it more obvious that this is for testing only?

Hmm, this is just the same pattern that's also used for mouse events when testing. It's set through
WKPageSetShouldSendEventsSynchronously from WTR.
 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:512
> > +    void touchEventSyncForTesting(const WebTouchEvent&, bool&);
> 
> it is not obvious what the bool means here. I would add 'handled'

Yeah, it's handled. I copied the signature from mouseEventSyncForTesting, but I can add the handled.
 
> > Tools/WebKitTestRunner/EventSenderProxy.h:54
> > +    void updateTouchPoint(int index, int x, int y);
> 
> isn;t it more an id than an index? At least Lars told me that iOS uses ids and not indices

No, this one is actually an index, not the touch point id.

The API works like this:

addTouchPoint(10, 10); // gets index 0
addTouchPoint(20, 20); // second touch point at index 1
touchStart();
updateTouchPoint(1, 30, 30); // Move second touch point to 30, 30
...
 
> > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:325
> > +    if (index < 0 || index >= m_touchPoints.count())
> 
> Shouldn't this be an assert?

Hmm, I guess so. I'll change it :)

> > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:363
> > +    for (int i = 0; i < m_touchPoints.count(); ++i)
> > +        if (m_touchPoints[i].state() != Qt::TouchPointReleased) {
> > +            sendTouchEvent(QEvent::TouchUpdate);
> > +            return;
> > +        }
> 
> Bad coding style... add braces

Uh, I agree.

[Simon Hausmann]

Created attachment 113295 [details]
[Qt][WK2] Add support for touch event testing to WebKitTestRunner

[Kenneth Rohde Christiansen]

Comment on attachment 113295 [details]
[Qt][WK2] Add support for touch event testing to WebKitTestRunner

Great stuff, r=me

[Simon Hausmann]

Comment on attachment 113295 [details]
[Qt][WK2] Add support for touch event testing to WebKitTestRunner

Clearing flags on attachment: 113295

Committed r99051: <http://trac.webkit.org/changeset/99051>

[Simon Hausmann]

All reviewed patches have been landed.  Closing bug.