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>
(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".
Created attachment 90618 [details] Patch
Simon, how safe is to ignore the "event->buttons() & Qt::LeftButton" statement here? same for Qt::RightButton and Qt::MidButton...
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 on attachment 90618 [details] Patch OK.
Comment on attachment 90618 [details] Patch Sounds like folks want tot alk to simon. removing cq+.
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 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.
Created attachment 93160 [details] Patch Updated patch based on Benjamin Poulain's comments.
Comment on attachment 93160 [details] Patch Attachment 93160 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8688422
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 on attachment 93160 [details] Patch Clearing review flags while I look to update the patch.
(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.
Created attachment 93191 [details] Patch
Comment on attachment 93191 [details] Patch Attachment 93191 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8685516
Created attachment 93197 [details] Patch
Comment on attachment 93197 [details] Patch Attachment 93197 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8688483
Created attachment 93198 [details] Patch
Comment on attachment 93198 [details] Patch Attachment 93198 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8689463
Created attachment 93204 [details] Patch
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
(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.
Committed r86381: <http://trac.webkit.org/changeset/86381>