Bug 98931 – [GTK] Enable touch features

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

This bug is meant to cover any addition of the touch support.

[Carlos Garnacho]

Created attachment 219912 [details]
[GTK] Add touch events to WK2

This patch uses GdkEventTouch events available in GTK+ >= 3.4 in order to implement web touch events, emulation if pointer events on the right situations is also implemented, so interacting on a WebKitWebView with touch devices works at least like it currently does through GTK+ builtin pointer emulation. TBH interaction with touch devices might still improve greatly (scrolling, zooming, and whatnot), but one thing at a time...

[Martin Robinson]

Nice work! I recommend uploading the patch with webkit-patch because it will point out all style errors before it hits the bug tracker.

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Commit Bot]

Attachment 219912 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformGTK.cmake', u'Source/WebCore/bindings/gobject/GNUmakefile.am', u'Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp', u'Source/WebCore/platform/gtk/GtkTouchContextHelper.h', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/PlatformGTK.cmake', u'Source/WebKit2/Shared/NativeWebTouchEvent.h', u'Source/WebKit2/Shared/gtk/NativeWebTouchEventGtk.cpp', u'Source/WebKit2/Shared/gtk/WebEventFactory.cpp', u'Source/WebKit2/Shared/gtk/WebEventFactory.h', u'Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp', u'Source/WebKit2/UIProcess/API/gtk/PageClientImpl.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp', u'Source/autotools/SetupWebKitFeatures.m4', u'Source/cmake/OptionsGTK.cmake', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:41:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:49:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:54:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:211:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:212:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:218:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:244:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:252:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:258:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:260:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:261:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:267:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:273:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:276:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:281:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:282:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:342:  Declaration has space between type name and * in const GdkEvent *touch_event  [whitespace/declaration] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:347:  Declaration has space between type name and * in GdkEvent *pointer_event  [whitespace/declaration] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:350:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:359:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:361:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:365:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:374:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:377:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:378:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:379:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebKit2/Shared/NativeWebTouchEvent.h:53:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WebCore/ChangeLog:11:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:727:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 31 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 219912 [details]
[GTK] Add touch events to WK2

Attachment 219912 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/49128213

[Carlos Garnacho]

Oh, heh :), too used to C coding styles, I'll update the patch and check errors

[Carlos Garcia Campos]

Comment on attachment 219912 [details]
[GTK] Add touch events to WK2

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.

[Carlos Garnacho]

Created attachment 219943 [details]
A more polished patch, passes style checks locally

[Carlos Garnacho]

Created attachment 219947 [details]
third patch, minor fix this time

[Carlos Garnacho]

Thanks KaL for the comments! All style issues are fixed, I'm replying below to some things

(In reply to comment #7)
> > 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?

Hmm well, GdkEventSequence is essentially a GINT_TO_POINTER(), and web touch events also prefer ints as IDs, I thought it made sense to do the same all through.

> > 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

I guess you mean a reference on the container? I kind of liked not having to copy GdkEvents around there :)

> > 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);

Indeed, refactored that out in my last patch

[Martin Robinson]

Comment on attachment 219947 [details]
third patch, minor fix this time

View in context: https://bugs.webkit.org/attachment.cgi?id=219947&action=review

Thanks for updating the patch. I have a few, mostly stylistic, nits.

> Source/WebCore/PlatformGTK.cmake:631
> +    if (ENABLE_TOUCH_EVENTS)
> +        list(APPEND GObjectDOMBindings_IDL_FILES
> +            dom/Touch.idl
> +        )
> +    endif ()
> +

We are enabling touch events unconditionally for GTK+ and this conditional isn't mirrored in the autotools version of the build, so I think it makes sense to unconditionally add the file to the list here as well.

Thank you very kindly for updating the CMake build!

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

The asterisk here is on the wrong side.

> Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:64
> +Vector<GdkEvent*> GtkTouchContextHelper::touchEvents(void) const
> +{
> +    Vector<GdkEvent*> touchEvents;
> +    copyValuesToVector(m_touchEvents, touchEvents);
> +    return touchEvents;
> +}
> +

Perhaps you could just do:

HashMapValuesProxy& GtkTouchContextHelper::touchEvents() const
{
    return m_touchEvents.values();
}

That would avoid the copy. WebKit uses empty argument lists instead of void.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:207
> +#ifndef GTK_API_VERSION_2

No need for this check here, since WebKit2 is only available for GTK+ 3 builds.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:232
> +    gdouble root_x, root_y;

Please use rootX and rootY here.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:272
> +    return WebTouchEvent(type, touchPointList, modifiersForEvent(event), (double) gdk_event_get_time(event));

Please use a static_cast here instead of the C style cast. Does an implicit cast work?

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:346
> +    const GdkEvent* touch_event = event.nativeEvent();
> +
> +    if (!touch_event->touch.emulating_pointer)
> +        return;

touch_event -> touchEvent.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:349
> +

pointer_event -> pointerEvent.

[Carlos Garnacho]

Thanks for the review Martin, only a few comments below so far:

(In reply to comment #11)
> (From update of attachment 219947 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219947&action=review
> 
> Thanks for updating the patch. I have a few, mostly stylistic, nits.
> 
> > Source/WebCore/PlatformGTK.cmake:631
> > +    if (ENABLE_TOUCH_EVENTS)
> > +        list(APPEND GObjectDOMBindings_IDL_FILES
> > +            dom/Touch.idl
> > +        )
> > +    endif ()
> > +
> 
> We are enabling touch events unconditionally for GTK+ and this conditional isn't mirrored in the autotools version of the build, so I think it makes sense to unconditionally add the file to the list here as well.

hmm, Isn't that done on the change to Source/autotools/SetupWebKitFeatures.m4? or should I be doing that somewhere else?

> 
> Thank you very kindly for updating the CMake build!

:), that was the part that I did somewhat more blindly tbh

> 
> > Source/WebKit2/Shared/gtk/WebEventFactory.cpp:207
> > +#ifndef GTK_API_VERSION_2
> 
> No need for this check here, since WebKit2 is only available for GTK+ 3 builds.

Isn't that code used on the webprocess? I got errors when compiling otherwise, from what I checked it was linking to gtk+2 at that time.

> > Source/WebKit2/Shared/gtk/WebEventFactory.cpp:272
> > +    return WebTouchEvent(type, touchPointList, modifiersForEvent(event), (double) gdk_event_get_time(event));
> 
> Please use a static_cast here instead of the C style cast. Does an implicit cast work?
> 

Right, saw other similar pieces of code casting implicitly, will update that

[Martin Robinson]

Comment on attachment 219947 [details]
third patch, minor fix this time

View in context: https://bugs.webkit.org/attachment.cgi?id=219947&action=review

>>> Source/WebCore/PlatformGTK.cmake:631
>>> +
>> 
>> We are enabling touch events unconditionally for GTK+ and this conditional isn't mirrored in the autotools version of the build, so I think it makes sense to unconditionally add the file to the list here as well.
>> 
>> Thank you very kindly for updating the CMake build!
> 
> hmm, Isn't that done on the change to Source/autotools/SetupWebKitFeatures.m4? or should I be doing that somewhere else?

I only meant that you shouldn't need to check for ENABLE_TOUCH_EVENTS since we are turning it on always. That does remind me that you should change the setting in Source/cmake/OptionsGTK.cmake as well though.

>>> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:207
>>> +#ifndef GTK_API_VERSION_2
>> 
>> No need for this check here, since WebKit2 is only available for GTK+ 3 builds.
> 
> Isn't that code used on the webprocess? I got errors when compiling otherwise, from what I checked it was linking to gtk+2 at that time.

Ah! A few files are compiled into the plugin process as well, which does indeed compile against GTK+2.

[Zan Dobersek]

Comment on attachment 219947 [details]
third patch, minor fix this time

View in context: https://bugs.webkit.org/attachment.cgi?id=219947&action=review

> Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:35
> +GtkTouchContextHelper::GtkTouchContextHelper()
> +{
> +}

This constructor is not necessary.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:222
> +    } else
> +        return WebPlatformTouchPoint::TouchStationary;
> +
> +    ASSERT_NOT_REACHED();
> +    return WebPlatformTouchPoint::TouchStationary;

You can remove the `else` branch and ASSERT_NOT_REACHED call and just return WebPlatformTouchPoint::TouchStationary when the event sequences differ.

[Carlos Garnacho]

Created attachment 219967 [details]
Updated patch

This new patch applies suggestions in comments #11 and #14, adds new spawned symbols to .symbols file, and fixes dubious memory management of GdkEvents in the helper

[Carlos Garnacho]

(In reply to comment #11)
> Perhaps you could just do:
> 
> HashMapValuesProxy& GtkTouchContextHelper::touchEvents() const
> {
>     return m_touchEvents.values();
> }
> 
> That would avoid the copy. WebKit uses empty argument lists instead of void.

I tried that, but HashMapValuesProxy seems private to HashMap, so the type is unknown there, I've finally resorted to returning the HashMap, and have the caller iterate over values.

[Carlos Garcia Campos]

Comment on attachment 219967 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219967&action=review

It looks much better, I have a just a few more minor comments. I also wonder if this makes tests in fast/events/touch pass, this bug is listed in the TestExpectations file, so I would expect a patch here to also unskip touch those tests (at least for WebKit2 tests in this case)

> Source/WebCore/platform/gtk/GtkTouchContextHelper.h:30
> +#include <wtf/Vector.h>

This is no longer needed now that you are returning the HashMap

> Source/WebCore/platform/gtk/GtkTouchContextHelper.h:37
> +class GtkTouchContextHelper {
> +public:

We probably want to make this non copyable. You only need to add here (before the public:) WTF_MAKE_NONCOPYABLE(GtkTouchContextHelper);

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

I don't understand why this is needed.

> Source/WebCore/platform/gtk/GtkTouchContextHelper.h:42
> +    const HashMap<uint32_t, GdkEvent*>& touchEvents() const { return m_touchEvents; };

I would use a typedef for this, something like

typedef TouchEventsMap HashMap<uint32_t, GdkEvent*>;

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:208
> +

Nit: remove this empty line here.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:239
> +

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:722
> +    WebKitWebViewBase* webViewBase = WEBKIT_WEB_VIEW_BASE(widget);
> +    WebKitWebViewBasePrivate* priv = webViewBase->priv;

We are only using priv, this could be just:

WebKitWebViewBasePrivate* priv = WEBKIT_WEB_VIEW_BASE(widget)->priv;

[Carlos Garnacho]

Created attachment 219970 [details]
updated patch

Further suggestions applied, and I also added code to implement missing touch events on EventSenderProxy, I've been however unable to run any test, so couldn't really tell if events in fast/events/touch are passing as it'd be expected.

[Carlos Garnacho]

Created attachment 219971 [details]
minor update, fix touch event leak in EventSenderProxyGtk

[WebKit Commit Bot]

Attachment 219971 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformGTK.cmake', u'Source/WebCore/bindings/gobject/GNUmakefile.am', u'Source/WebCore/bindings/gobject/webkitdom.symbols', u'Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp', u'Source/WebCore/platform/gtk/GtkTouchContextHelper.h', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/PlatformGTK.cmake', u'Source/WebKit2/Shared/NativeWebTouchEvent.h', u'Source/WebKit2/Shared/gtk/NativeWebTouchEventGtk.cpp', u'Source/WebKit2/Shared/gtk/WebEventFactory.cpp', u'Source/WebKit2/Shared/gtk/WebEventFactory.h', u'Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp', u'Source/WebKit2/UIProcess/API/gtk/PageClientImpl.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp', u'Source/autotools/SetupWebKitFeatures.m4', u'Source/cmake/OptionsGTK.cmake', u'Tools/ChangeLog', u'Tools/WebKitTestRunner/EventSenderProxy.h', u'Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp', '--commit-queue']" exit_code: 1
ERROR: Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:520:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garnacho]

Created attachment 219972 [details]
another patch fixing an style issue that was introduced

I'll stop churning patches now :)

[Carlos Garnacho]

Created attachment 219984 [details]
a last patch hopefully, fixes silly mistake

[Carlos Garnacho]

Created attachment 223095 [details]
Updated patch, minor fixups and updates test situation

I've finally went through all hops awaiting in LayoutTests/, In order to run tests fine with this patch, the patches in bugs #128170 and #128171 need to be applied first.

Even though, I had to expect failures for some touch tests in GTK+, as they were testing platform specific behavior that's hard and pointless to replicate here, I've filed bug #128172 about that.

[WebKit Commit Bot]

Attachment 223095 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:519:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:548:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebKit2/Shared/gtk/WebEventFactory.cpp:266:  Declaration has space between type name and * in GdkEvent *storedEvent  [whitespace/declaration] [3]
Total errors found: 3 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garnacho]

Created attachment 223096 [details]
patch, fixing style...

[Carlos Garnacho]

Created attachment 223121 [details]
rebased patch

[Carlos Garcia Campos]

Comment on attachment 223121 [details]
rebased patch

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 :-)

[Xabier Rodríguez Calvar]

Comment on attachment 223121 [details]
rebased patch

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.

[Xabier Rodríguez Calvar]

(In reply to comment #28)
> > 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.

If you do that, to enable ON by default, you need to change Source/cmake/WebKitFeatures.cmake

[Carlos Garnacho]

Created attachment 223391 [details]
Updated patch, applies most suggestions

[Carlos Garnacho]

I applied all suggestions, just a few comments below

(In reply to comment #27)
> > 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.

Yes. I did the bare minimum to have things compiling in WK1. That said, it should work
just like always wrt touch events, just knowing about pointer emulation.

> > Tools/DumpRenderTree/gtk/EventSender.cpp:917
> > +    return JSValueMakeUndefined(context);
> 
> We don't need to do anything here?

IIRC I had to add that so WK1 compiled after I added the touch IDLs to build, shouldn't be ever triggered.

(In reply to comment #28)
> > Source/WebCore/platform/gtk/GtkTouchContextHelper.h:41
> > +    GtkTouchContextHelper() { };
> 
> Can't this constructor go as Zan says?
> 

I tried and no dice, WTF_MAKE_NONCOPYABLE seems to like having a constructor

> > 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.

I tried being more verbose there :), but actually not a lot happens there, just plenty of data structure wrapping.

> 
> > Source/WebKit2/PlatformGTK.cmake:570
> > +        Shared/WebTouchEvent.cpp
> 
> Incorrectly sorted.
> 
> Also, Shared/WebPlatformTouchPoint.cpp is missing here, otherwise, it won't link.

Good catch :)

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

Yup, did that on WebKitFeatures.cmake

[Carlos Garnacho]

Created attachment 223469 [details]
patch

Minor fix. This patch uses HashSet on EventSenderProxy to track the updated events, so the code is easier to follow.

[Martin Robinson]

Comment on attachment 223469 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223469&action=review

> Source/cmake/OptionsEfl.cmake:-92
> -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_TOUCH_EVENTS ON)

I think this has the side-effect of enabling this for WinCE, so you should probably just switch the port default for GTK+.

> Source/cmake/WebKitFeatures.cmake:112
> -    WEBKIT_OPTION_DEFINE(ENABLE_TOUCH_EVENTS "Toggle Touch Events support" OFF)
> +    WEBKIT_OPTION_DEFINE(ENABLE_TOUCH_EVENTS "Toggle Touch Events support" ON)

See above.

[Carlos Garnacho]

Created attachment 223472 [details]
patch, turn back to GTK+-only config options

[Carlos Garnacho]

Created attachment 223617 [details]
patch, add missing symbol to webkitdom.symbols

About the gtk-wk1 build failing in the previous patch, it looked like another case of older files in DerivedSources/WebCore/... but it does build fine here

[Carlos Garcia Campos]

(In reply to comment #35)
> Created an attachment (id=223617) [details]
> patch, add missing symbol to webkitdom.symbols
> 
> About the gtk-wk1 build failing in the previous patch, it looked like another case of older files in DerivedSources/WebCore/... but it does build fine here

It has happened again, I tried the patch as well locally and it built.

[Carlos Garcia Campos]

Comment on attachment 223617 [details]
patch, add missing symbol to webkitdom.symbols

Ok, it seems EWS bots need a clean build or something.

[WebKit Commit Bot]

Comment on attachment 223617 [details]
patch, add missing symbol to webkitdom.symbols

Clearing flags on attachment: 223617

Committed r163749: <http://trac.webkit.org/changeset/163749>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Zan Dobersek]

Comment on attachment 223617 [details]
patch, add missing symbol to webkitdom.symbols

View in context: https://bugs.webkit.org/attachment.cgi?id=223617&action=review

Landed a couple of fixes that were crashing the related tests in 
http://trac.webkit.org/changeset/163774

> Source/WebCore/platform/gtk/GtkTouchContextHelper.cpp:40
> +    case GDK_TOUCH_BEGIN: {
> +        ASSERT(m_touchEvents.contains(sequence));

This ASSERT now checks for the opposite, which is what was most likely intended.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:475
> +    m_touchEvents.append(std::move(event));
> +    m_updatedTouchEvents.add(GPOINTER_TO_INT(event->touch.sequence));

std::move() moves the event object into the vector and invalidates the local variable, so dereferencing that variable caused crashes.