[webkit-reviews] review denied: [Bug 48510] [GTK] Implement WebEventFactory, WebErrors classes for WebKit2 : [Attachment 82016] Proposed patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 48510: [GTK] Implement WebEventFactory, WebErrors classes for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=48510

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

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

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:33
> +

No newline needed here.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:111
> +    int sClickCount = 0;
> +
> +    switch (event->type) {
> +    case GDK_BUTTON_PRESS:
> +	   sClickCount = 1;
> +	   break;
> +    case GDK_2BUTTON_PRESS:
> +	   sClickCount = 2;
> +	   break;
> +    case GDK_3BUTTON_PRESS:
> +	   sClickCount = 3;
> +	   break;
> +    case GDK_MOTION_NOTIFY:
> +    case GDK_BUTTON_RELEASE:
> +	   sClickCount = 0;
> +	   break;

This code is very suspicious. We need to support click counts higher than 3 and
also to detect up front situations where GDK is sending both a GDK_BUTTON_PRESS
followed closely by a GDK_3BUTTON_PRESS/GDK_2BUTTON_PRESS. See the code in
webkitwebview.cpp to do this. My guess is that we'll need similar code in the
widget and to pass in the click count manually.

Please don't use Hungarian notation (sClickCount).

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:139
> +    GdkEventButton* bEvent;

Please do not abbreviate. Use buttonEvent or eventButton.

> Source/WebKit2/Shared/gtk/WebEventFactory.h:32
> +#include <gdk/gdk.h>

Please use typedefs instead of including gdk.h here.

> Source/WebKit2/Shared/gtk/WebEventFactory.h:37
> +    public:

No indentation needed here.