Bug 58863 – [Qt] fast/events/fire-mousedown-while-pressing-mouse-button.html failing

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

The layout test fast/events/fire-mousedown-while-pressing-mouse-button.html committed in changeset <http://trac.webkit.org/changeset/84217> is failing on the Qt Linux Release bot. In particular, it fails the following tests:

press middle mouse button while pressing the left mouse button
press right mouse button while pressing the left mouse button
press right middle mouse button while pressing the right mouse button

For completeness, the layout tests results are here:
<http://build.webkit.org/builders/Qt%20Linux%20Release/builds/31600/steps/layout-test/logs/stdio>
<http://build.webkit.org/results/Qt%20Linux%20Release/r84218%20(31600)/results.html>
<http://build.webkit.org/results/Qt%20Linux%20Release/r84218%20(31600)/fast/events/fire-mousedown-while-pressing-mouse-button-pretty-diff.html>

[Daniel Bates]

(In reply to comment #0)
> press right middle mouse button while pressing the right mouse button

I meant "press middle mouse button while pressing the right mouse button".

[Daniel Bates]

Created attachment 90618 [details]
Patch

[Antonio Gomes]

Simon, how safe is to ignore the "event->buttons() & Qt::LeftButton" statement here? same for Qt::RightButton and Qt::MidButton...

[Antonio Gomes]

Comment on attachment 90618 [details]
Patch

Lets wait for Simon. I think it is correct, but it might have implications that I do not know of ...

[Eric Seidel (no email)]

Comment on attachment 90618 [details]
Patch

OK.

[Eric Seidel (no email)]

Comment on attachment 90618 [details]
Patch

Sounds like folks want tot alk to simon.  removing cq+.

[Daniel Bates]

Created attachment 92589 [details]
Patch

Updated patch. For mouse move events we need to use the bitmask returned by QMouseEvent::buttons() instead of the return value of QMouseEvent::button() because QMouseEvent::button() returns Qt::NoButton as per <http://doc.qt.nokia.com/latest/qmouseevent.html#button>.

[Benjamin Poulain]

Comment on attachment 92589 [details]
Patch

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

Make sense, just check this:

> Source/WebCore/platform/qt/PlatformMouseEventQt.cpp:38
> +static MouseButton mouseButtonForQtMouseButtons(Qt::MouseButtons mouseButtons)
> +{

I would prefer this to take the event as the parameter.
This way, the test "m_eventType == MouseEventMoved ? event->buttons() : event->button()" can be done in this function instead of being repeated by the callers.

[Daniel Bates]

Created attachment 93160 [details]
Patch

Updated patch based on Benjamin Poulain's comments.

[Early Warning System Bot]

Comment on attachment 93160 [details]
Patch

Attachment 93160 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8688422

[Benjamin Poulain]

Comment on attachment 93160 [details]
Patch

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

I don't r+ yet because I am curious about the FIXME. I like the change otherwise, the new handling looks cleaner than the old code.
I will look at the fixme at work.

> Source/WebCore/platform/qt/PlatformMouseEventQt.cpp:3
> + * Copyright (C) 2011 Research In Motion Limited.

hehe

> Source/WebCore/platform/qt/PlatformMouseEventQt.cpp:102
> +    #ifndef QT_NO_CONTEXTMENU

The preprocessor commands should be aligned on the left (due to some pretty bad compiler on some exotic platform supported by Qt :))

> Source/WebCore/platform/qt/PlatformMouseEventQt.cpp:113
> +    #endif
> +    if (!isContextMenuEvent) {
> +        mouseEventTypeAndMouseButtonFromQEvent(event, m_eventType, m_button);

I think you can get rid of isContextMenuEvent and simplify.
The idea
#ifdef
   if() {
   } else
#endif
   {
       ...
   }

Your choose if it is simpler or not to have to bool :)

[Daniel Bates]

Comment on attachment 93160 [details]
Patch

Clearing review flags while I look to update the patch.

[Daniel Bates]

(In reply to comment #11)
> (From update of attachment 93160 [details])
> [...]
> > Source/WebCore/platform/qt/PlatformMouseEventQt.cpp:102
> > +    #ifndef QT_NO_CONTEXTMENU
> 
> The preprocessor commands should be aligned on the left (due to some pretty bad compiler on some exotic platform supported by Qt :))

Will fix.

> 
> > Source/WebCore/platform/qt/PlatformMouseEventQt.cpp:113
> > +    #endif
> > +    if (!isContextMenuEvent) {
> > +        mouseEventTypeAndMouseButtonFromQEvent(event, m_eventType, m_button);
> 
> I think you can get rid of isContextMenuEvent and simplify.
> The idea
> #ifdef
>    if() {
>    } else
> #endif
>    {
>        ...
>    }

Notice we want to determine the mouse event type and button when compiling with context menu support (i.e.  "#ifndef QT_NO_CONTEXTMENU" evaluates to true) and event->type() != QEvent::ContextMenu. I'm unclear from your code snippet if this will work out with the structure you mentioned.

[Daniel Bates]

Created attachment 93191 [details]
Patch

[Early Warning System Bot]

Comment on attachment 93191 [details]
Patch

Attachment 93191 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8685516

[Daniel Bates]

Created attachment 93197 [details]
Patch

[Early Warning System Bot]

Comment on attachment 93197 [details]
Patch

Attachment 93197 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8688483

[Daniel Bates]

Created attachment 93198 [details]
Patch

[Early Warning System Bot]

Comment on attachment 93198 [details]
Patch

Attachment 93198 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8689463

[Daniel Bates]

Created attachment 93204 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 93204 [details]
Patch

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

Pretty nice!

> Source/WebCore/platform/qt/PlatformMouseEventQt.cpp:114
> +    // FIXME: Why don't we handle a context menu event here as we do in PlatformMouseEvent(QInputEvent*, int)?

Maybe Benjamin would know... he dealt a bit with that for webkit2

[Daniel Bates]

(In reply to comment #21)
> (From update of attachment 93204 [details])
> > Source/WebCore/platform/qt/PlatformMouseEventQt.cpp:114
> > +    // FIXME: Why don't we handle a context menu event here as we do in PlatformMouseEvent(QInputEvent*, int)?
> 
> Maybe Benjamin would know... he dealt a bit with that for webkit2

From speaking with Benjamin today on IRC, he couldn't think of any reason why such a difference exists between the PlatformMouseEvent constructors. Filed bug #60728 for this issue.

[Daniel Bates]

Committed r86381: <http://trac.webkit.org/changeset/86381>