Bug 58863

Summary: [Qt] fast/events/fire-mousedown-while-pressing-mouse-button.html failing
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Tools / TestsAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, hausmann, kling, ossy, tonikitoo
Priority: P3 Keywords: LayoutTestFailure, Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
eric: review+
Patch
benjamin: review+, benjamin: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch kenneth: review+

Description Daniel Bates 2011-04-18 21:19:44 PDT
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>
Comment 1 Daniel Bates 2011-04-18 21:31:25 PDT
(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".
Comment 2 Daniel Bates 2011-04-21 15:41:19 PDT
Created attachment 90618 [details]
Patch
Comment 3 Antonio Gomes 2011-04-25 17:02:51 PDT
Simon, how safe is to ignore the "event->buttons() & Qt::LeftButton" statement here? same for Qt::RightButton and Qt::MidButton...
Comment 4 Antonio Gomes 2011-04-25 17:23:59 PDT
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 ...
Comment 5 Eric Seidel (no email) 2011-05-01 12:31:50 PDT
Comment on attachment 90618 [details]
Patch

OK.
Comment 6 Eric Seidel (no email) 2011-05-01 12:32:20 PDT
Comment on attachment 90618 [details]
Patch

Sounds like folks want tot alk to simon.  removing cq+.
Comment 7 Daniel Bates 2011-05-06 09:29:24 PDT
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>.
Comment 8 Benjamin Poulain 2011-05-10 14:44:54 PDT
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.
Comment 9 Daniel Bates 2011-05-11 12:44:21 PDT
Created attachment 93160 [details]
Patch

Updated patch based on Benjamin Poulain's comments.
Comment 10 Early Warning System Bot 2011-05-11 12:53:23 PDT
Comment on attachment 93160 [details]
Patch

Attachment 93160 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8688422
Comment 11 Benjamin Poulain 2011-05-11 14:16:14 PDT
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 :)
Comment 12 Daniel Bates 2011-05-11 14:25:36 PDT
Comment on attachment 93160 [details]
Patch

Clearing review flags while I look to update the patch.
Comment 13 Daniel Bates 2011-05-11 15:06:51 PDT
(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.
Comment 14 Daniel Bates 2011-05-11 15:11:14 PDT
Created attachment 93191 [details]
Patch
Comment 15 Early Warning System Bot 2011-05-11 15:24:26 PDT
Comment on attachment 93191 [details]
Patch

Attachment 93191 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8685516
Comment 16 Daniel Bates 2011-05-11 15:33:09 PDT
Created attachment 93197 [details]
Patch
Comment 17 Early Warning System Bot 2011-05-11 15:43:05 PDT
Comment on attachment 93197 [details]
Patch

Attachment 93197 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8688483
Comment 18 Daniel Bates 2011-05-11 15:46:11 PDT
Created attachment 93198 [details]
Patch
Comment 19 Early Warning System Bot 2011-05-11 15:55:01 PDT
Comment on attachment 93198 [details]
Patch

Attachment 93198 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8689463
Comment 20 Daniel Bates 2011-05-11 16:00:09 PDT
Created attachment 93204 [details]
Patch
Comment 21 Kenneth Rohde Christiansen 2011-05-12 01:39:25 PDT
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
Comment 22 Daniel Bates 2011-05-12 14:41:46 PDT
(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.
Comment 23 Daniel Bates 2011-05-12 14:43:58 PDT
Committed r86381: <http://trac.webkit.org/changeset/86381>