[Qt] DRT doesn't support double click simulation

[Charles Wei]

2  consecutive mouse click ((MouseDown, MouseUp, MouseDown, MouseUp) should generate a Double Click event.

[Charles Wei]

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]

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 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]

Created attachment 41013 [details]
[QT] DRT double-click simulation with 2 consecutive mouse click

[Charles Wei]

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]

(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 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]

Created attachment 41105 [details]
resubmit after adding reset and leapForward

[Charles Wei]

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]

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 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]

(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]

I raised the review flag just to get the attention :-)

[Kenneth Rohde Christiansen]

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 @@
> [phone]  Charles Wei  <[e-mail]>
> +
> +        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]

No updates on this?!

[Jesus Sanchez-Palencia]

Just talked to Diego and it seems that now DRT has double click support.
Closing this bug as fixed.