WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
30271
[Qt] DRT doesn't support double click simulation
https://bugs.webkit.org/show_bug.cgi?id=30271
Summary
[Qt] DRT doesn't support double click simulation
Charles Wei
Reported
2009-10-10 00:54:13 PDT
2 consecutive mouse click ((MouseDown, MouseUp, MouseDown, MouseUp) should generate a Double Click event.
Attachments
[QT] DumpRenderTree double click simulation with 2 consecutive mouse click event
(3.95 KB, patch)
2009-10-10 01:12 PDT
,
Charles Wei
zecke
: review-
Details
Formatted Diff
Diff
[QT] DRT double-click simulation with 2 consecutive mouse click
(4.12 KB, patch)
2009-10-11 20:33 PDT
,
Charles Wei
zimmermann
: review-
Details
Formatted Diff
Diff
resubmit after adding reset and leapForward
(5.14 KB, patch)
2009-10-13 09:01 PDT
,
Charles Wei
kenneth
: review-
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Charles Wei
Comment 1
2009-10-10 01:12:29 PDT
Created
attachment 40985
[details]
[QT] DumpRenderTree double click simulation with 2 consecutive mouse click event On Qt platform , for a double click mouse event , the event sequence sent to the application is like this : MouseButtonPress, MouseButtonRelease, MouseButtonDblClick, MouseButtonRelease So in the DumpRenderTree, if the JavaScript test code is : EventSender.mouseDown(button); EventSender.mouseUp(button); EventSender.mouseDown(button); EventSender.mouseUp(button); The event sequence mentioned above should be send to the webkit, and in event handler for MousebuttonDblClick, an MouseButtonPressEvent will be automatically emitted inside webkit.
Jakub Wieczorek
Comment 2
2009-10-10 03:27:23 PDT
It seems that: eventSender.mouseDown(0); // left button eventSender.mouseUp(0); eventSender.mouseDown(2); // right button eventSender.mouseUp(2); would simulate a double click event too with this patch. Looking at the logic from Mac's EventSender I assume this should not happen: - (void)updateClickCountForButton:(int)buttonNumber { if (([self currentEventTime] - lastClick >= 1) || !NSEqualPoints(lastMousePosition, lastClickPosition) || lastClickButton != buttonNumber) { clickCount = 1; lastClickButton = buttonNumber; } else clickCount++; } Resetting m_timeStarted to false in LTC::reset() may make sense too. In theory running two (or more) subsequent tests that do not move the mouse position and simulate two single clicks would create a double click event in the second test.
Holger Freyther
Comment 3
2009-10-10 06:41:14 PDT
Comment on
attachment 40985
[details]
[QT] DumpRenderTree double click simulation with 2 consecutive mouse click event I think Jakub's comment make sense and the patch should be changed.
Charles Wei
Comment 4
2009-10-11 20:33:03 PDT
Created
attachment 41013
[details]
[QT] DRT double-click simulation with 2 consecutive mouse click
Charles Wei
Comment 5
2009-10-11 20:45:32 PDT
Thanks for the review , Jakub. The new patch addresses your comments about a double click event will be generated with 2 different button clicks, which is incorrect. About your other comments "Resetting m_timeStarted to false in LTC::reset() may make sense too. In theory running two (or more) subsequent tests that do not move the mouse position and simulate two single clicks would create a double click event in the second test." I don't agree with that . 1. class LayoutTestController and EventSender are two totally different classes which don't have any connection , it makes no sense to reset EventSender inside LayoutTestController. 2. EventServer, as an Javascript object, will be instantiated for each test page, two or more subsequent tests will each has its own instance of EventSender, so mouse clicks in 2 test cases won't create a double click event in the second test, instead, 2 single-clicks will be created , one for each test case . --Charles
Jakub Wieczorek
Comment 6
2009-10-12 10:34:43 PDT
(In reply to
comment #5
)
> 2. EventServer, as an Javascript object, will be instantiated for each test > page, two or more subsequent tests will each has its own instance of > EventSender, so mouse clicks in 2 test cases won't create a double click event > in the second test, instead, 2 single-clicks will be created , one for each > test case .
Actually, all the controller objects, that are exposed to JS, are created only once in each DRT instance, which is reused as long as possible until one of the tests crashes or a specific limit of subsequent tests is reached. Qt DRT uses the QObject-JSC bridge and it needs to readd the relevant objects to the window object after it is cleared but it does not reinstantiate them.
> 1. class LayoutTestController and EventSender are two totally different classes > which don't have any connection , it makes no sense to reset EventSender inside > LayoutTestController.
Yes, I agree. How about DRT::resetToConsistentStateBeforeTesting()? (if that means making DRT a friend of EventSender, this should be ok I guess). I believe it may be a good idea to look at the layout tests that Qt is passing and that use eventSender.leapForward() (which is not implemented in Qt DRT) and check if there are any regressions. Now that the EventSender would support double click events, the lack of that feature might introduce some new failures. fast/events/frame-click-focus.html might be one example. I'm not a reviewer so please take it all as suggestions.
Nikolas Zimmermann
Comment 7
2009-10-12 10:43:30 PDT
Comment on
attachment 41013
[details]
[QT] DRT double-click simulation with 2 consecutive mouse click I'm not familiar with the Qt-DRT code, though Jakub comments makes sense IMHO. Giving r- to make it disappear from the review queue. Please investigate regarding leapForward(), or wheter this breaks any other tests? (Though I suppose you already checked running layout tests, that nothing else is broken?)
Charles Wei
Comment 8
2009-10-13 09:01:49 PDT
Created
attachment 41105
[details]
resubmit after adding reset and leapForward
Charles Wei
Comment 9
2009-10-13 09:16:09 PDT
To Jakub: Thanks, Jakub. I re-investigated the DumpRenderTree.cpp, and yes, you are right , the EventSender will only be instantiated once for one run of DRT, then we need to reset the state of EventSender for each test case to avoid the wrong mouse event sent. that has been addressed with EventSender::reset(). Thanks . leapForward(int ms) is implemented to just idle for that much of time. the leapFoward() can make 2 seperate clicks or a double click, according to the time interval that was leap forwarded. I ran run-webkit-test , didn't find any regression. Actually this patch makes one test case a success which used to fail.
Charles Wei
Comment 10
2009-10-13 18:53:37 PDT
To Jakub: Thanks, Jakub. I re-investigated the DumpRenderTree.cpp, and yes, you are right , the EventSender will only be instantiated once for one run of DRT, then we need to reset the state of EventSender for each test case to avoid the wrong mouse event sent. that has been addressed with EventSender::reset(). Thanks . leapForward(int ms) is implemented to just idle for that much of time. the leapFoward() can make 2 seperate clicks or a double click, according to the time interval that was leap forwarded. I ran run-webkit-test , didn't find any regression. Actually this patch makes one test case a success which used to fail.
Eric Seidel (no email)
Comment 11
2009-10-19 14:25:36 PDT
Comment on
attachment 41105
[details]
resubmit after adding reset and leapForward m_timeStarted is not a clear variable name. I don't know what it's supposed to do. Why? + usleep(ms * 1000); That seems like a very bad idea.
Charles Wei
Comment 12
2009-10-19 19:28:36 PDT
(In reply to
comment #11
)
> (From update of
attachment 41105
[details]
) > m_timeStarted is not a clear variable name. I don't know what it's supposed to > do. > > Why? > + usleep(ms * 1000); > > That seems like a very bad idea.
m_timeStarted serves like an indicator to test if double click should be tested. like for the first mouse click , you don't need to test if it's a double click; a click after a double click should not be tested as a double click , etc. about usleep(ms * 1000) for leapForward(int ms), leapFoward is supposed to simulate the time flow by ms (in millisecond), and mostly for double click simulation or prevents a double click with a longer leapFoward time. like below: EventSender.mouseDown(0); EventSender.mouseUp(0); EventSender.leapForward(1000); // without this, a dblclick will be generated EventSender.mouseDown(0); EventSender.mouseUp(0); If sleep for the ms indicated by leapFoward(ms) is a bad idea, please tell me a better idea so that I can implement. I don't know what to do with just a comment like "a bad idea".
Charles Wei
Comment 13
2009-10-19 19:30:02 PDT
I raised the review flag just to get the attention :-)
Kenneth Rohde Christiansen
Comment 14
2009-11-10 04:59:50 PST
Comment on
attachment 41105
[details]
resubmit after adding reset and leapForward
> Index: WebKitTools/ChangeLog > =================================================================== > --- WebKitTools/ChangeLog (revision 49502) > +++ WebKitTools/ChangeLog (working copy) > @@ -1,3 +1,20 @@ > +2009-10-13 Charles Wei <
charles.wei@torchmobile.com.cn
> > + > + Reviewed by NOBODY (OOPS!). > + > + [QT] Double Click Simulation support for DRT
Could be more descriptive
> Index: WebKitTools/DumpRenderTree/qt/jsobjects.cpp > =================================================================== > --- WebKitTools/DumpRenderTree/qt/jsobjects.cpp (revision 49496) > +++ WebKitTools/DumpRenderTree/qt/jsobjects.cpp (working copy) > @@ -373,6 +373,14 @@ EventSender::EventSender(QWebPage *paren > : QObject(parent) > { > m_page = parent; > + m_timeStarted = false;
m_timerIsRunning? I agree with Eric, it is very hard understanding what this is used for
> + > + if (!m_timeStarted || m_lastButton != mouseButton || m_mouseTime.elapsed() > QApplication::doubleClickInterval()) { > + m_mouseTime.restart();
Is this a timestamp? then call it so.
> void EventSender::leapForward(int ms) > { > - m_timeLeap += ms; > + usleep(ms * 1000); > qDebug() << "EventSender::leapForward" << ms; > }
Maybe instead of using usleep (which will make our tests run slower) just modify the timestamps of the mouse events? like (m_mouseTime.elapsed() + m_leapTime) > QApplication::doubleClickInterval() Also I guess reset() should reset the m_leapTime?
Jesus Sanchez-Palencia
Comment 15
2010-05-11 13:05:39 PDT
No updates on this?!
Jesus Sanchez-Palencia
Comment 16
2010-05-11 13:11:08 PDT
Just talked to Diego and it seems that now DRT has double click support. Closing this bug as fixed.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug