[webkit-reviews] review granted: [Bug 68556] [WK2] [Qt] Implement MouseDown/MouseUp/MouseMoveTo functions for WebKit2 EventSender : [Attachment 108502] patch 1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chang Shu <cshu at webkit.org>'s request
for review:
Bug 68556: [WK2] [Qt] Implement MouseDown/MouseUp/MouseMoveTo functions for
WebKit2 EventSender
https://bugs.webkit.org/show_bug.cgi?id=68556

Attachment 108502: patch 1
https://bugs.webkit.org/attachment.cgi?id=108502&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.