[webkit-reviews] review denied: [Bug 98931] [GTK] Add touch support : [Attachment 219912] [GTK] Add touch events to WK2

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 98931: [GTK] Add touch support
https://bugs.webkit.org/show_bug.cgi?id=98931

Attachment 219912: [GTK] Add touch events to WK2
https://bugs.webkit.org/attachment.cgi?id=219912&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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.