[webkit-reviews] review granted: [Bug 69411] [WK2] [GTK] Implement a MouseDown/MouseUp/MouseMoveTo/MouseScrollBy/LeapForward functions for WebKit2 EventSender : [Attachment 114100] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 69411: [WK2] [GTK] Implement a
MouseDown/MouseUp/MouseMoveTo/MouseScrollBy/LeapForward functions for WebKit2
EventSender
https://bugs.webkit.org/show_bug.cgi?id=69411

Attachment 114100: Patch
https://bugs.webkit.org/attachment.cgi?id=114100&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114100&action=review


Looks good, but please resolve the issues below before landing.

> LayoutTests/platform/gtk-wk2/Skipped:43
> +editing/pasteboard/smart-drag-drop.html
> +editing/selection/5195166-1.html
> +editing/selection/resources/select-all-iframe-src.html
> +fast/events/resources/drag-outside-window-frame.html
> +fast/frames/resources/frame-dead-region-left.html
> +fast/loader/resources/click-fragment-link.html
> +fast/loader/resources/early-load-cancel-inner.html
> +fast/loader/resources/document-with-fragment-url-test.html
> +plugins/resources/resize-from-plugin-frame.html
> +plugins/plugin-initiate-popup-window.html

This seems fishy. The results are already generated for WebKit1 aren't they. If
these simply differ from WebKit1 results, then either the tests are failing
here or in WebKit1.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:61
> +    WTREventQueueItem(): event(0), delay(0) { }

The style for this needs to be:

WTREventQeueuItem()
    : event(0),
    , delay(0)
{
}

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:62
> +    WTREventQueueItem(GdkEvent* e, gulong d) : event(e), delay(d) { }

Ditto and e should be named event and d should be named delay.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:296
> +void EventSenderProxy::mouseDown(unsigned button, WKEventModifiers
wkModifiers)

Shouldn't this method check whether the mouse down event has already happened
like the WK1 event sender? See EventSender line 319. We need to be careful to
not throw away all the knowledge we've gained from the first EventSender!

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:308
> +    updateClickCountForButton(button);
> +
> +    if (m_clickCount == 2)
> +	   eventType = GDK_2BUTTON_PRESS;
> +    else if (m_clickCount == 3)
> +	   eventType = GDK_3BUTTON_PRESS;
> +    else
> +	   eventType = GDK_BUTTON_PRESS;
> +

There was a huge comment in the WebKit1 EventSender here.  I think we should
preserve it.

    // Normally GDK will send both GDK_BUTTON_PRESS and GDK_2BUTTON_PRESS for
    // the second button press during double-clicks. WebKit GTK+ selectively
    // ignores the first GDK_BUTTON_PRESS of that pair using gdk_event_peek.
    // Since our events aren't ever going onto the GDK event queue, WebKit
won't
    // be able to filter out the first GDK_BUTTON_PRESS, so we just don't send
    // it here. Eventually this code should probably figure out a way to get
all
    // appropriate events onto the event queue and this work-around should be
    // removed.