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

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied 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 113870: Patch
https://bugs.webkit.org/attachment.cgi?id=113870&action=review

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


Thanks for cleanup Sergio. Couple nits here, but I think this is a big
improvement.

> LayoutTests/platform/gtk-wk2/Skipped:69
> +# FAIL: Timed out waiting for notifyDone to be called

There's already a section of the Skipped file for tests that time out. You
should probably just move this section there.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:56
> +struct WTREventQueue {
> +    GdkEvent* event;
> +    gulong delay;
> +};
> +
> +static WTREventQueue msgQueue[1024];
> +static unsigned endOfQueue;
> +static unsigned startOfQueue;

This should be a member of EventSenderProxy as well. Instead of using an array
you should use a Vector. It should probably be named eventQueue, but at the
very least messageQueue.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:69
> +    , m_position()

I'm pretty sure you can omit this. The default constructor is called by
default.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:73
> +    , m_clickPosition()

Ditto.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:111
> +    // Map EventSender button to actual GDK Mouse button.

You can omit this comment entirely. The function name epresses the same idea.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:144
> +    // First send all the events that are ready to be sent

This comment is missing a period.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:168
>  static guint getModifiers(WKEventModifiers modifiersRef)

Please rename this to webkitModifiersToGKDModifiers to match the other
conversion functions. I know it's unrelated, but this is a good opportunity for
it. Also modifiersRef isn't actually a reference, so it should be renamed as
well. wkModifiers is probably fine.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:326
> +    // Depending on click count change the type of button event.
> +    if (m_clickCount == 2)
> +	   event->type = GDK_2BUTTON_PRESS;
> +    else if (m_clickCount == 3)
> +	   event->type = GDK_3BUTTON_PRESS;
> +
> +    createMouseButtonEvent(event, getModifiers(modifiers));
> +    sendOrQueueEvent(event);
> +}
> +
> +void EventSenderProxy::mouseUp(unsigned button, WKEventModifiers modifiers)
> +{
> +    // Create initially Single click button event.
> +    GdkEvent* event = gdk_event_new(GDK_BUTTON_RELEASE);
> +    event->button.button = eventSenderButtonToGDKButton(button);
> +
> +    createMouseButtonEvent(event, getModifiers(modifiers));
> +    sendOrQueueEvent(event);

It seems that createMouseButtonEvent should take care of all of this. I think
it should work such that you call it like this:

GdkEvent* event = createMouseButtonEvent(GDK_BUTTON_RELEASE, button,
modifiers);
sendOrQueueEvent(event);
and then:
GdkEvent* event = createMouseButtonEvent(GDK_BUTTON_PRESS, button, modifiers);
sendOrQueueEvent(event);

functions that have "create" in the name are assumed to be factories.