Bug 71310 - [Qt][WK2] Add support for touch event testing to WebKitTestRunner
Summary: [Qt][WK2] Add support for touch event testing to WebKitTestRunner
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Hausmann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-01 12:39 PDT by Simon Hausmann
Modified: 2011-11-02 02:57 PDT (History)
3 users (show)

See Also:


Attachments
Patch work in progress (23.40 KB, patch)
2011-11-01 12:47 PDT, Simon Hausmann
no flags Details | Formatted Diff | Diff
[Qt][WK2] Add support for touch event testing to WebKitTestRunner (32.32 KB, patch)
2011-11-02 02:01 PDT, Simon Hausmann
no flags Details | Formatted Diff | Diff
[Qt][WK2] Add support for touch event testing to WebKitTestRunner (32.30 KB, patch)
2011-11-02 02:07 PDT, Simon Hausmann
no flags Details | Formatted Diff | Diff
[Qt][WK2] Add support for touch event testing to WebKitTestRunner (32.38 KB, patch)
2011-11-02 02:48 PDT, Simon Hausmann
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2011-11-01 12:39:22 PDT
[Qt][WK2] Add support for touch event testing to WebKitTestRunner
Comment 1 Simon Hausmann 2011-11-01 12:47:35 PDT
Created attachment 113205 [details]
Patch work in progress
Comment 2 Simon Hausmann 2011-11-02 02:01:28 PDT
Created attachment 113290 [details]
[Qt][WK2] Add support for touch event testing to WebKitTestRunner
Comment 3 WebKit Review Bot 2011-11-02 02:04:37 PDT
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.
Comment 4 Simon Hausmann 2011-11-02 02:07:05 PDT
Created attachment 113292 [details]
[Qt][WK2] Add support for touch event testing to WebKitTestRunner
Comment 5 Kenneth Rohde Christiansen 2011-11-02 02:15:24 PDT
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
Comment 6 Simon Hausmann 2011-11-02 02:44:44 PDT
(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.
Comment 7 Simon Hausmann 2011-11-02 02:48:59 PDT
Created attachment 113295 [details]
[Qt][WK2] Add support for touch event testing to WebKitTestRunner
Comment 8 Kenneth Rohde Christiansen 2011-11-02 02:50:41 PDT
Comment on attachment 113295 [details]
[Qt][WK2] Add support for touch event testing to WebKitTestRunner

Great stuff, r=me
Comment 9 Simon Hausmann 2011-11-02 02:57:36 PDT
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>
Comment 10 Simon Hausmann 2011-11-02 02:57:44 PDT
All reviewed patches have been landed.  Closing bug.