[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