Bug 98931

Summary: [GTK] Enable touch features
Product: WebKit Reporter: Zan Dobersek <zan>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, bunhere, calvaris, carlosg, cdumez, cgarcia, commit-queue, dbates, eflews.bot, gustavo, gyuyoung.kim, hugo.lima, mrobinson, rakuco, sergio, william.jon.mccann
Priority: P2 Keywords: Gtk, LayoutTestFailure
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 128170, 128171, 128250    
Bug Blocks:    
Attachments:
Description Flags
[GTK] Add touch events to WK2
cgarcia: review-, eflews.bot: commit-queue-
A more polished patch, passes style checks locally
none
third patch, minor fix this time
none
Updated patch
none
updated patch
none
minor update, fix touch event leak in EventSenderProxyGtk
none
another patch fixing an style issue that was introduced
none
a last patch hopefully, fixes silly mistake
none
Updated patch, minor fixups and updates test situation
none
patch, fixing style...
none
rebased patch
none
Updated patch, applies most suggestions
none
patch
none
patch, turn back to GTK+-only config options
none
patch, add missing symbol to webkitdom.symbols none

Description Zan Dobersek 2012-10-10 11:29:01 PDT
This bug is meant to cover any addition of the touch support.
Comment 1 Carlos Garnacho 2013-12-23 05:53:21 PST
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...
Comment 2 Martin Robinson 2013-12-23 08:36:21 PST
Nice work! I recommend uploading the patch with webkit-patch because it will point out all style errors before it hits the bug tracker.
Comment 3 WebKit Commit Bot 2013-12-23 08:37:30 PST
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
Comment 4 WebKit Commit Bot 2013-12-23 08:37:39 PST
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.
Comment 5 EFL EWS Bot 2013-12-23 08:44:56 PST
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
Comment 6 Carlos Garnacho 2013-12-23 08:47:02 PST
Oh, heh :), too used to C coding styles, I'll update the patch and check errors
Comment 7 Carlos Garcia Campos 2013-12-23 11:05:35 PST
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.
Comment 8 Carlos Garnacho 2013-12-23 17:00:27 PST
Created attachment 219943 [details]
A more polished patch, passes style checks locally
Comment 9 Carlos Garnacho 2013-12-23 17:18:51 PST
Created attachment 219947 [details]
third patch, minor fix this time
Comment 10 Carlos Garnacho 2013-12-23 17:19:42 PST
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
Comment 11 Martin Robinson 2013-12-23 17:33:38 PST
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.
Comment 12 Carlos Garnacho 2013-12-23 18:14:39 PST
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
Comment 13 Martin Robinson 2013-12-23 18:18:41 PST
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.
Comment 14 Zan Dobersek 2013-12-24 01:38:45 PST
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.
Comment 15 Carlos Garnacho 2013-12-24 05:47:24 PST
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
Comment 16 Carlos Garnacho 2013-12-24 05:49:29 PST
(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.
Comment 17 Carlos Garcia Campos 2013-12-24 07:09:52 PST
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;
Comment 18 Carlos Garnacho 2013-12-24 08:49:59 PST
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.
Comment 19 Carlos Garnacho 2013-12-24 08:59:49 PST
Created attachment 219971 [details]
minor update, fix touch event leak in EventSenderProxyGtk
Comment 20 WebKit Commit Bot 2013-12-24 09:01:11 PST
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.
Comment 21 Carlos Garnacho 2013-12-24 09:02:31 PST
Created attachment 219972 [details]
another patch fixing an style issue that was introduced

I'll stop churning patches now :)
Comment 22 Carlos Garnacho 2013-12-24 16:34:08 PST
Created attachment 219984 [details]
a last patch hopefully, fixes silly mistake
Comment 23 Carlos Garnacho 2014-02-04 03:54:34 PST
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.
Comment 24 WebKit Commit Bot 2014-02-04 03:57:34 PST
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.
Comment 25 Carlos Garnacho 2014-02-04 04:10:05 PST
Created attachment 223096 [details]
patch, fixing style...
Comment 26 Carlos Garnacho 2014-02-04 08:19:41 PST
Created attachment 223121 [details]
rebased patch
Comment 27 Carlos Garcia Campos 2014-02-04 09:50:00 PST
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 :-)
Comment 28 Xabier Rodríguez Calvar 2014-02-05 08:14:54 PST
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.
Comment 29 Xabier Rodríguez Calvar 2014-02-05 08:31:50 PST
(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
Comment 30 Carlos Garnacho 2014-02-06 15:19:16 PST
Created attachment 223391 [details]
Updated patch, applies most suggestions
Comment 31 Carlos Garnacho 2014-02-06 15:36:23 PST
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
Comment 32 Carlos Garnacho 2014-02-07 10:33:58 PST
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.
Comment 33 Martin Robinson 2014-02-07 10:38:57 PST
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.
Comment 34 Carlos Garnacho 2014-02-07 10:50:44 PST
Created attachment 223472 [details]
patch, turn back to GTK+-only config options
Comment 35 Carlos Garnacho 2014-02-09 01:45:28 PST
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
Comment 36 Carlos Garcia Campos 2014-02-09 02:13:32 PST
(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.
Comment 37 Carlos Garcia Campos 2014-02-09 02:21:30 PST
Comment on attachment 223617 [details]
patch, add missing symbol to webkitdom.symbols

Ok, it seems EWS bots need a clean build or something.
Comment 38 WebKit Commit Bot 2014-02-09 02:50:53 PST
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>
Comment 39 WebKit Commit Bot 2014-02-09 02:50:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 40 Zan Dobersek 2014-02-10 01:13:29 PST
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.