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

[bugzilla-daemon at webkit.org]

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





--- Comment #27 from Carlos Garcia Campos <cgarcia at igalia.com>  2014-02-04 09:50:00 PST ---
(From update of attachment 223121)
View in context: https://bugs.webkit.org/attachment.cgi?id=223121&action=review

Look good to me in general, I still have a few comments.

> LayoutTests/ChangeLog:8
> +        * platform/gtk/TestExpectations: remove fast/events/touch

Do these tests pass in wk1 now? if touch support is wk2 only, they should still be skipped in wk1

> LayoutTests/platform/gtk-wk2/TestExpectations:189
> +fast/events/touch [ Pass ]

If they pass, you can simply remove this :-)

> Source/WebCore/bindings/gobject/GNUmakefile.am:266
> +	DerivedSources/webkitdom/WebKitDOMTouch.cpp \
> +	DerivedSources/webkitdom/WebKitDOMTouchPrivate.h \

I wonder what happens with this API in wk1, I guess it simply won't work.

> Source/WebCore/bindings/gobject/GNUmakefile.am:438
> +

This extra line looks unneeded.

> Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:47
> +    GdkEvent* event = m_touchEvents.take(sequence);
> +    if (event)
> +        gdk_event_free(event);

You should use GUniquePtr for this. That way you could use remove instead of take.

> Source/WebCore/platform/gtk/GtkTouchContextHelper.h:36
> +typedef HashMap<uint32_t, GdkEvent*> TouchEventsMap;

This could be typedef HashMap<uint32_t, GUniquePtr<GdkEvent>> TouchEventsMap;

> Source/WebKit2/Shared/NativeWebTouchEvent.h:37
> +typedef union _GdkEvent GdkEvent;

This is no longer needed now, because GUniquePtrGtk.h includes gdk

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:244
> +    TouchEventsMap touchEvents = touchContext.touchEvents();

const TouchEventsMap& ? or even auto touchEvents = touchContext.touchEvents();

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:267
> +        GdkEvent* storedEvent = it->value;
> +        appendTouchEvent(touchPointList, storedEvent, touchPhaseFromEvents(storedEvent, event));

This could be just one line

appendTouchEvent(touchPointList, it->value, touchPhaseFromEvents(it->value, event));

> Tools/DumpRenderTree/gtk/EventSender.cpp:917
> +    return JSValueMakeUndefined(context);

We don't need to do anything here?

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:460
> +    touchEvent->touch.sequence = (GdkEventSequence*) GINT_TO_POINTER(id);

Use C++ style cast

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:471
> +    GdkEvent* event = createTouchEvent((GdkEventType) GDK_TOUCH_BEGIN, m_touchEvents.size() + 1);

Ditto.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:520
> +    for (unsigned i = 0; i < m_touchEvents.size(); ++i)
> +        gdk_event_free(m_touchEvents[i]);

I think this could be something like:

for (auto event : m_touchEvents)
    gdk_event_free(event);

But now that we have GUniquePtr, we could use GUniquePtrGtk.h here, since it's implemented in the header and you could use it to define the Vector as something like Vector<GUniquePtr<GdkEvent>> m_touchEvents and then you wouldn't need the for loop.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:549
> +    for (unsigned i = 0; i < m_touchEvents.size(); ++i) {
> +        GdkEvent* event = m_touchEvents[i];

This could probably be a modern loop too :-)

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