Bug 68556 - [WK2] [Qt] Implement MouseDown/MouseUp/MouseMoveTo functions for WebKit2 EventSender
Summary: [WK2] [Qt] Implement MouseDown/MouseUp/MouseMoveTo functions for WebKit2 Even...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chang Shu
URL:
Keywords:
Depends on: 68108
Blocks: 42194
  Show dependency treegraph
 
Reported: 2011-09-21 12:02 PDT by Chang Shu
Modified: 2011-09-24 09:02 PDT (History)
4 users (show)

See Also:


Attachments
patch 1 (19.62 KB, patch)
2011-09-23 11:45 PDT, Chang Shu
darin: review+
Details | Formatted Diff | Diff
patch 2: addressed review comments (19.96 KB, patch)
2011-09-23 13:35 PDT, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chang Shu 2011-09-21 12:02:27 PDT
The Qt port based on bug 68108
Comment 1 Chang Shu 2011-09-23 11:45:56 PDT
Created attachment 108502 [details]
patch 1
Comment 2 Darin Adler 2011-09-23 12:36:53 PDT
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.
Comment 3 Chang Shu 2011-09-23 13:28:04 PDT
> > 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.
Comment 4 Chang Shu 2011-09-23 13:35:35 PDT
Created attachment 108526 [details]
patch 2: addressed review comments

moved operator== to WKGeometry.h
Comment 5 WebKit Review Bot 2011-09-23 16:01:07 PDT
Comment on attachment 108526 [details]
patch 2: addressed review comments

Clearing flags on attachment: 108526

Committed r95878: <http://trac.webkit.org/changeset/95878>
Comment 6 WebKit Review Bot 2011-09-23 16:01:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Xan Lopez 2011-09-24 02:46:46 PDT
This patch broke GTK+ and Windows bots. Apparently the actually committed patch (#2?) didn't go through EWS for some reason...
Comment 8 Alejandro G. Castro 2011-09-24 02:58:15 PDT
Trying to fix GTK compilation, operator== was copied but not removed from the original definition:

http://trac.webkit.org/changeset/95908
Comment 9 Chang Shu 2011-09-24 09:02:02 PDT
Sorry for the broken build and thanks for the fix. It's weird though the bot didn't detect the problem.