[Webkit-unassigned] [Bug 98931] [GTK] Add touch support

[bugzilla-daemon at webkit.org]

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


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #219912|review?                     |review-
               Flag|                            |




--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-12-23 11:05:35 PST ---
(From update of attachment 219912)
View in context: https://bugs.webkit.org/attachment.cgi?id=219912&action=review

Thanks for the patch, I've made a few more comments apart form the styles issues mentioned by the style bot.

> ChangeLog:6
> +        set ENABLE_TOUCH_EVENTS to 1 if building with GTK+
> +        https://bugs.webkit.org/show_bug.cgi?id=98931

Please move the bug url right below the bug title, and the bug description after the Reviewed by line.

> Source/WebCore/ChangeLog:7
> +        Building and linking to that code is necessary if the GTK code is to implement
> +        touch events.
> +        https://bugs.webkit.org/show_bug.cgi?id=98931

Ditto.

> Source/WebCore/ChangeLog:11
> +        No new tests (OOPS!).

This should be removed if the patch doesn't affect any layout test.

> Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:28
> +#include "GtkTouchContextHelper.h"
> +#include <gtk/gtk.h>

There should be a line between these.

> Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:36
> +bool GtkTouchContextHelper::handle(GdkEvent *touchEvent)

This could be handleTouchEvent or just handleEvent

> Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:38
> +#if GTK_CHECK_VERSION(3, 4, 0)

We depend on gtk 3.6, if this is not available in gtk2 use GTK_API_VERSION_2 instead.

> Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:41
> +    uint32_t sequence;
> +
> +    sequence = GPOINTER_TO_UINT (gdk_event_get_event_sequence (touchEvent));

This should be a single line. Why are we converting it to an integer?

> Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:52
> +        return FALSE;

FALSE -> false

> Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:56
> +    return TRUE;

TRUE -> true

> Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:59
> +Vector<GdkEvent*> GtkTouchContextHelper::getTouchEvents(void) const

For methods returning a value the name should be without the get, just GtkTouchContextHelper::touchEvents(). However, in this case we could leave the name and make the function receive a reference

> Source/WebCore/platform/gtk/GtkTouchContextHelper.h:29
> +#include "GRefPtrGtk.h"

This doesn't seem to be used.

> Source/WebKit/gtk/ChangeLog:8
> +        Need the bug URL (OOPS!).

Bug url is missing. you can use Tools/Scripts/prepare-ChangeLog script to create the changelog entries.

> Source/WebKit2/ChangeLog:9
> +        In GTK+ >= 3.4.0, GdkEventTouch is available to inform about multitouch events. Use these to implement
> +        touch events on this platform. If a touch is left unhandled and is the "pointer emulating" one, mouse
> +        events will be generated as a fallback.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=98931

Same comments here.

> Source/WebKit2/Shared/NativeWebTouchEvent.h:53
> +    NativeWebTouchEvent(GdkEvent*,WebCore::GtkTouchContextHelper&);

Leave a space between ,WebCore::

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:207
> +#if GTK_CHECK_VERSION(3, 4, 0)

This is not needed, we depend on gtk 3.6 and wk2 doesn't support gtk2

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:214
> +        else if (current->type == GDK_TOUCH_BEGIN)

Don't use else after a return.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:218
> +    } else {
> +        return WebPlatformTouchPoint::TouchStationary;
> +    }

Don't uses braces here.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:248
> +    Vector<GdkEvent*> touchEvents;
> +    touchEvents = touchContext.getTouchEvents();

Use single line here, unless we change the function to receive a reference as I suggested before.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:249
> +    touchPointList.reserveInitialCapacity(touchEvents.size() + 1);

Move the touchPointList delclaration here, when it's first used.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:262
> +        WebPlatformTouchPoint touchPoint(identifier, touchPhaseFromEvents (event, touchEvents[i]),
> +                                         IntPoint(root_x, root_y), IntPoint(x, y));
> +        touchPointList.uncheckedAppend(touchPoint);

This can probably be a single line:

touchPointList.uncheckedAppend(WebPlatformTouchPoint(identifier, touchPhaseFromEvents(event, touchEvents[i]), IntPoint(root_x, root_y), IntPoint(x, y)));

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:266
> +        // Touch was already removed from the GtkTouchContextHelper, add it here

Nit: Finish the comments with a period.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:277
> +        touchPointList.uncheckedAppend(touchPoint);

This is mostly the same than previous block, we can probably make a helper function like appendTouchEvent(Vector<WebPlatformTouchPoint>&, GdkEvent*, WebPlatformTouchPoint::TouchPointState);

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:340
> +#if GTK_CHECK_VERSION(3, 4, 0)

This is not needed either.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:341
> +    // emulate pointer events if unhandled

Nit: start comments with capital letter and finish with period.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:347
> +    GdkEvent *pointer_event;

Use GOwnPtr here.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:360
> +        if (touch_event->type == GDK_TOUCH_BEGIN) {
> +            pointer_event = gdk_event_new (GDK_BUTTON_PRESS);
> +        } else if (touch_event->type == GDK_TOUCH_END) {

Don't use braces for single statement ifs. You can probably use a switch here instead.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:378
> +    gdk_event_free (pointer_event);

You don't need this when using GOwnPtr.

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