[Webkit-unassigned] [Bug 98931] [GTK] Enable touch features

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Feb 5 08:17:38 PST 2014


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





--- Comment #28 from Xabier Rodríguez Calvar <calvaris at igalia.com>  2014-02-05 08:14:54 PST ---
(From update of attachment 223121)
View in context: https://bugs.webkit.org/attachment.cgi?id=223121&action=review

I think I could do an informal review that should be reviewed by another reviewer. I hope you can make some use of my comments.

> Source/WebCore/PlatformGTK.cmake:201
> +    platform/gtk/GtkTouchContextHelper.cpp

Incorrectly sorted.

> Source/WebCore/PlatformGTK.cmake:216
> +    platform/gtk/PlatformTouchEventGtk.cpp

Why are you including this file? It doesn't seem to be in the build and it will break it. (And it would be incorrectly sorted).

> Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:36
> +    for (auto it = m_touchEvents.begin(); it != m_touchEvents.end(); ++it)
> +        gdk_event_free(it->value);

I guess you can avoid the loop if using GUniquePtr.

> Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:55
> +    GdkEvent* event = m_touchEvents.take(sequence);
> +    if (event)
> +        gdk_event_free(event);
> +
> +    switch (touchEvent->type) {
> +    case GDK_TOUCH_BEGIN:
> +    case GDK_TOUCH_UPDATE:
> +        m_touchEvents.set(sequence, gdk_event_copy(touchEvent));
> +        break;
> +    case GDK_TOUCH_END:
> +        break;

If you use GUniquePtr you can avoid the take and just use remove in the _END.

> Source/WebCore/platform/gtk/GtkTouchContextHelper.h:41
> +    GtkTouchContextHelper() { };

Can't this constructor go as Zan says?

> Source/WebKit2/ChangeLog:31
> +        * GNUmakefile.list.am:
> +        * PlatformGTK.cmake:
> +        * Shared/gtk/NativeWebTouchEventGtk.cpp:
> +        * Shared/NativeWebTouchEvent.h:
> +        (WebKit::NativeWebTouchEvent::nativeEvent):
> +        (WebKit::NativeWebTouchEvent::touchContext): Add GTK+ implementation of NativeWebTouchEvent
> +        * Shared/gtk/WebEventFactory.cpp:
> +        (WebKit::touchPhaseFromEvents):
> +        (WebKit::WebEventFactory::createWebTouchEvent): Add methods to generate NativeWebTouchEvents out
> +        of GdkEventTouch events, a GtkTouchContextHelper object is used to hold information about all current
> +        touches, in order to build information about all individual touchpoints.
> +        * Shared/gtk/WebEventFactory.h:
> +        * UIProcess/API/gtk/PageClientImpl.cpp:
> +        (WebKit::PageClientImpl::doneWithTouchEvent): Implement pointer emulation if the pointer emulating
> +        touch was unhandled.
> +        * UIProcess/API/gtk/PageClientImpl.h:
> +        * UIProcess/API/gtk/WebKitWebViewBase.cpp:
> +        (webkitWebViewBaseRealize): Listen for touch events
> +        (webkitWebViewBaseTouchEvent):
> +        (webkit_web_view_base_class_init): Add implementation for the touch_events() handler.

I think this could be a bit more descriptive.

> Source/WebKit2/PlatformGTK.cmake:60
> +    Shared/gtk/NativeWebTouchEventGtk.cpp

Incorrectly sorted.

> Source/WebKit2/PlatformGTK.cmake:570
> +        Shared/WebTouchEvent.cpp

Incorrectly sorted.

Also, Shared/WebPlatformTouchPoint.cpp is missing here, otherwise, it won't link.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:243
> +    WebEvent::Type type = static_cast<WebEvent::Type>(0);

What about NoType instead of 0?

> Source/cmake/OptionsGTK.cmake:69
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_TOUCH_EVENTS ON)

If both EFL and GTK have now ENABLE_TOUCH_EVENTS ON we could remove this line and the one for EFL.

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


More information about the webkit-unassigned mailing list