Created attachment 108502 [details] patch 1

Comment on attachment 108502 [details] patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=108502&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1054 > + handled = false; Another way to do this is to just write: handled = m_pageOverlay && m_pageOverlay->mouseEvent(mouseEvent); One reason I like that is that the code already relies on handled not being set before the m_pageOverlay check, because there is no "if (!handled)" around it. > Tools/WebKitTestRunner/EventSenderProxy.h:74 > +inline bool operator==(const WKPoint& a, const WKPoint& b) > +{ > + return a.x == b.x && a.y == b.y; > +} This function is fine, but it is in the wrong file. OK for now, but I think it probably belongs in WebKit2 itself. Need to find a better header to put it in. One possibility is to have it in the public API file that has WKPoint itself, with #ifdef __cplusplus around it. > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:255 > +void EventSenderProxy::updateClickCountForButton(int button) > +{ > + if (m_time - m_clickTime < 1 && m_position == m_clickPosition && button == m_clickButton) { > + ++m_clickCount; > + m_clickTime = m_time; > + return; > + } > + > + m_clickCount = 1; > + m_clickTime = m_time; > + m_clickPosition = m_position; > + m_clickButton = button; > +} This seems like it might be platform-independent. The "< 1" thing seems like it needs a “why” comment.

> > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:255 > > +void EventSenderProxy::updateClickCountForButton(int button) > > +{ > > + if (m_time - m_clickTime < 1 && m_position == m_clickPosition && button == m_clickButton) { > > + ++m_clickCount; > > + m_clickTime = m_time; > > + return; > > + } > > + > > + m_clickCount = 1; > > + m_clickTime = m_time; > > + m_clickPosition = m_position; > > + m_clickButton = button; > > +} > > This seems like it might be platform-independent. The "< 1" thing seems like it needs a “why” comment. Actually, it should be QApplication::doubleClickInterval()/1000.0 on Qt. Thanks for pointing this out.

Created attachment 108526 [details] patch 2: addressed review comments moved operator== to WKGeometry.h

Comment on attachment 108526 [details] patch 2: addressed review comments Clearing flags on attachment: 108526 Committed r95878: <http://trac.webkit.org/changeset/95878>

All reviewed patches have been landed. Closing bug.

This patch broke GTK+ and Windows bots. Apparently the actually committed patch (#2?) didn't go through EWS for some reason...

Trying to fix GTK compilation, operator== was copied but not removed from the original definition: http://trac.webkit.org/changeset/95908

Sorry for the broken build and thanks for the fix. It's weird though the bot didn't detect the problem.