[Webkit-unassigned] [Bug 48510] [GTK] Implement WebEventFactory, WebErrors classes for WebKit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48510





--- Comment #18 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-02-11 00:10:42 PST ---
(From update of attachment 82016)
View in context: https://bugs.webkit.org/attachment.cgi?id=82016&action=review

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:40
> +static inline GdkPoint positionForEvent(GdkEventAny* event)

We don't need this, we can use gdk_event_get_coords()

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:67
> +static inline GdkPoint globalPositionForEvent(GdkEventAny* event)

We don't need this either, we can use gdk_event_get_root_coords()

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:113
> +    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;
> +    default:
> +        ASSERT_NOT_REACHED();

I would return the click count for every case instead of using a variable

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:114
> +    };

There's ; there

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:143
> +    case GDK_MOTION_NOTIFY:
> +        motion = (GdkEventMotion*) event;

You can avoid all the casts here by using event->motion.state passing GdkEvent * instead of GdkEventAny to this function

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:156
> +    case GDK_BUTTON_PRESS:
> +    case GDK_2BUTTON_PRESS:
> +    case GDK_3BUTTON_PRESS:
> +    case GDK_BUTTON_RELEASE:
> +        bEvent = (GdkEventButton *)event;

Same here using event->button.button

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:174
> +    guint timestamp = 0;

why not using event->time (you could use gdk_event_get_time()) instead of 0?

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:180
> +    GdkPoint globalPosition = globalPositionForEvent((GdkEventAny *)event);

event is already GdkEventAny, that cast is redundant. I recommend using GdkEvent instead of GdkEventAny anyway.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:257
> +    if ((state & GDK_SHIFT_MASK) || (state == GDK_KEY_3270_BackTab))

That's wrong, GDK_KEY_3270_BackTab is a keyval, not a state. I think we could use a single funcion modifiersForEvent() that receives an event instead of a state. Then gdk_event_get_state() and if event is a key press then check whether event->keyval is GDK_KEY_3270_BackTab

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.