While working on kinetic scrolling (bug #155750) I noticed that webkit doesnt have support for touch emulation on the view layer. Touch emulation is only supported in the WebCore layer (bug #77105), but the events there are not propagated to the view (only to the page), meaning testing things like kinetic scrolling (which requires touch events) wouldnt work without a real touchscreen device. As I didnt have a touchscreen device available, I decided to add a basic support (single touch) for emulation on the view, by sending synthetic touch events when receiving mouse events. The impl uses the GTK_TEST_TOUCHSCREEN env variable to decide whether to enable emulation (as already done by other official GTK+ widgets). While I believe this wont get integrated, I wanted to leave it here in case someone needs something similar in the future. Patch to come.
Created attachment 283592 [details] Patch
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 on attachment 283592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283592&action=review Why do you think we wouldn't want this? Supporting standard GTK+ touch event emulation certainly seems desirable. I'm not giving r+ myself because Carlos Garcia will probably want to look this over, though. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:232 > + gboolean touchEventEmulationInProgress; Since this isn't API, you should use nice C++ bools which actually guarantee the variable has one of two values. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:233 > + unsigned long touchEventSequence; Why the choice of unsigned long? I would probably use uint64_t for this. My general rule is to use int64_t when not sure whether the value might ever overflow, and int otherwise. If you're sure 32 bits is enough and are just trying to make sure it's not less than that on embedded devices, don't bother: glib isn't going to work on any system with less than 32-bit ints. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:240 > +static gboolean dispatchSyntheticTouchEventIfEnabled(GtkWidget*, GdkEvent*, GdkEventType); Again, not API, so you can use bool. Also, it's usually possible to rearrange the functions to avoid the forward declaration; if you can, please do so. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:577 > + priv->touchEventEmulationInProgress = FALSE; Lowercase true, false, false, since you'll change this to bool. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:851 > + return TRUE; I notice this condition will never be met; see my comment below. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1072 > + return FALSE; false > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1076 > + priv->touchEventEmulationInProgress = TRUE; true > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1088 > + GdkEventTouch* touchEvent = (GdkEventTouch*) gdk_event_new(eventType); No C-style casts. Also, use auto so you don't have to repeat the type on both sides of the expression: auto touchEvent = reinterpret_cast<GdkEventTouch*>(gdk_event_new(eventType)); > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1093 > + touchEvent->sequence = (GdkEventSequence*) GUINT_TO_POINTER(priv->touchEventSequence); Use static_cast here. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1104 > + return FALSE; This can't be right. This function always returns false!
(In reply to comment #3) > Why the choice of unsigned long? Ah sorry, I missed that you were casting it to a GdkEventSequence*. I'll presume that unsigned long is the right type for doing that.
Created attachment 283596 [details] Patch
Comment on attachment 283592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283592&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:232 >> + gboolean touchEventEmulationInProgress; > > Since this isn't API, you should use nice C++ bools which actually guarantee the variable has one of two values. Fixed. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:233 >> + unsigned long touchEventSequence; > > Why the choice of unsigned long? I would probably use uint64_t for this. My general rule is to use int64_t when not sure whether the value might ever overflow, and int otherwise. If you're sure 32 bits is enough and are just trying to make sure it's not less than that on embedded devices, don't bother: glib isn't going to work on any system with less than 32-bit ints. I changed it to guint as I am using GUINT_TO_POINTER to cast it to GdkEventSequence* anyway. It is used to hold an "address" for the event sequence which is an opaque struct. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:240 >> +static gboolean dispatchSyntheticTouchEventIfEnabled(GtkWidget*, GdkEvent*, GdkEventType); > > Again, not API, so you can use bool. > > Also, it's usually possible to rearrange the functions to avoid the forward declaration; if you can, please do so. Fixed the bool usage, but left the forward declaration as I would have to rearrange a bunch of functions do fix it. I can do it if you still prefer though. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:577 >> + priv->touchEventEmulationInProgress = FALSE; > > Lowercase true, false, false, since you'll change this to bool. Fixed. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:851 >> + return TRUE; > > I notice this condition will never be met; see my comment below. hehe, yeah, see below :). >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1072 >> + return FALSE; > > false Changed method to void, see below. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1076 >> + priv->touchEventEmulationInProgress = TRUE; > > true Fixed. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1088 >> + GdkEventTouch* touchEvent = (GdkEventTouch*) gdk_event_new(eventType); > > No C-style casts. Also, use auto so you don't have to repeat the type on both sides of the expression: > > auto touchEvent = reinterpret_cast<GdkEventTouch*>(gdk_event_new(eventType)); Ditto. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1093 >> + touchEvent->sequence = (GdkEventSequence*) GUINT_TO_POINTER(priv->touchEventSequence); > > Use static_cast here. Ditto. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1104 >> + return FALSE; > > This can't be right. This function always returns false! hehe, this was actually a leftover from when I was invoking webViewBaseTouchEvent directly instead of queueing the event with gdk_event_put :).
(In reply to comment #3) > Why do you think we wouldn't want this? Supporting standard GTK+ touch event > emulation certainly seems desirable. I'm not giving r+ myself because Carlos > Garcia will probably want to look this over, though. > Tbh, I had that workaround feeling when writing this patch, but guess I could be wrong :). And thanks for the review btw.
Comment on attachment 283596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283596&action=review Thanks for the patch, I also think this is useful, maybe we could keep the GTK+ naming and use test touch screen instead of emulare touch events. I have some comments and questions. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:231 > + bool emulateTouchEvent; Do we really need to keep this on every web view instance? I think we can use a helper function with a static local variable to cache this for all web views. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:233 > + guint touchEventSequence; guint -> unsigned > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:238 > +static guint webkitWebViewBaseGlobalTouchEventSequence = 0; Instead of a global variable use a function that returns the next global event sequence. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:240 > +static void dispatchSyntheticTouchEventIfEnabled(GtkWidget*, GdkEvent*, GdkEventType); Do not add prototypes, simply add the function implementation before it's used. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:578 > + priv->touchEventSequence = 0; What's the difference between this and the global webkitWebViewBaseGlobalTouchEventSequence? > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:851 > +#if ENABLE(TOUCH_EVENTS) > + dispatchSyntheticTouchEventIfEnabled(widget, reinterpret_cast<GdkEvent*>(buttonEvent), GDK_TOUCH_BEGIN); > +#endif Instead of adding the #ifdef everywhere, define the function unconditionally and add the #ifdef in the function body. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:885 > +#if ENABLE(TOUCH_EVENTS) > + dispatchSyntheticTouchEventIfEnabled(widget, reinterpret_cast<GdkEvent*>(event), GDK_TOUCH_END); > +#endif So, for every event, we still send the normal event to the web process, but we also synthesize an equivalent touch event? > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1069 > + if (!priv->emulateTouchEvent) > + return; So, what happens when the env variable is set and a real touch screen is used? I think we should check the actual event source and also return early when it's GDK_SOURCE_TOUCHSCREEN > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1074 > + priv->touchEventSequence = ++webkitWebViewBaseGlobalTouchEventSequence; Ah, this is the current sequence then. Can we use this also to see if there's an emulation in progress? when it's > 0. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1082 > + case GDK_TOUCH_UPDATE: > + case GDK_TOUCH_END: > + break; > + default: > + ASSERT_NOT_REACHED(); > + break; > + } I'm not sure a switch is useful here when you only want to do something when eventType == GDK_TOUCH_BEGIN > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1085 > + auto touchEvent = reinterpret_cast<GdkEventTouch*>(gdk_event_new(eventType)); You are leaking this event, use GUniquePtr > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1096 > + gdk_event_put(reinterpret_cast<GdkEvent*>(touchEvent)); Do we really need to synthesize events? Can't we just create a NativeWebTouchEvent in every case and send it to the web process? I also wonder if the GTK+ gesture controller doesn't handle the GTK_TEST_TOUCHSCREEN variable, so we could send the normal events to the gesture controller instead. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1156 > + WebKitWebViewBasePrivate* priv = WEBKIT_WEB_VIEW_BASE(widget)->priv; > + priv->touchEventEmulationInProgress = false; You should explain why you need to do this in the ChangeLog. > Source/WebKit2/UIProcess/gtk/GestureController.cpp:159 > + if (!gtk_gesture_handles_sequence(gesture, sequence)) { > + gtk_gesture_set_state(gesture, GTK_EVENT_SEQUENCE_DENIED); > + return; > + } > if (!dragGesture->m_inDrag) { > dragGesture->handleTap(gtk_gesture_get_last_event(gesture, sequence)); > gtk_gesture_set_state(gesture, GTK_EVENT_SEQUENCE_DENIED); > - } else if (!gtk_gesture_handles_sequence(gesture, sequence)) > - gtk_gesture_set_state(gesture, GTK_EVENT_SEQUENCE_DENIED); > + } Is this an existing issue or something needed for this patch?
Comment on attachment 283596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283596&action=review Thanks for the review, I will work on the suggestions. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:885 >> +#endif > > So, for every event, we still send the normal event to the web process, but we also synthesize an equivalent touch event? Yes, that is the current behaviour, see below. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1074 >> + priv->touchEventSequence = ++webkitWebViewBaseGlobalTouchEventSequence; > > Ah, this is the current sequence then. Can we use this also to see if there's an emulation in progress? when it's > 0. Actually, we cant use the sequence to check if there is an emulation in progress as it will always be > 0 after the first touch sequence happens (begin/update/end) while the boolean is false when we receive a touch end. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1096 >> + gdk_event_put(reinterpret_cast<GdkEvent*>(touchEvent)); > > Do we really need to synthesize events? Can't we just create a NativeWebTouchEvent in every case and send it to the web process? I also wonder if the GTK+ gesture controller doesn't handle the GTK_TEST_TOUCHSCREEN variable, so we could send the normal events to the gesture controller instead. Actually that is what I was doing before. My initial impl was invoking the webkitWebViewBaseTouchEvent callback directly instead of queueing the event and it would consume the mouse events when the callback returned TRUE - in which case things like text selection with mouse wouldnt work when running the touch emulation. The thing is that in this case we get different behaviours. The current patch here doesnt consume the mouse events, so text selection, for instance works in this case (not sure if desirable). I changed it as I thought we could potentially have issues when not queueing the event and processing it with the gesture controller - although it worked fine with the tests. So, please let me know which way do you prefer. >> Source/WebKit2/UIProcess/gtk/GestureController.cpp:159 >> + } > > Is this an existing issue or something needed for this patch? This is actually an issue I found when working on the kinetic scrolling support (bug #155750 - the last patch there includes this fix as part of my changes). I dont remember exactly what happened but iirc gtk_gesture_get_last_event would return NULL causing a crash on handleTap, meaning some other gesture handled the sequence (or it was cancelled...) while the DragGesture received a drag-begin/end. I can get a backtrace and open a new bug if needed for this but it is quite easy to reproduce the crash without this change - mix scrolling with dragging elements a few times and it blows away.
(In reply to comment #9) > Comment on attachment 283596 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=283596&action=review > > Thanks for the review, I will work on the suggestions. > > >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:885 > >> +#endif > > > > So, for every event, we still send the normal event to the web process, but we also synthesize an equivalent touch event? > > Yes, that is the current behaviour, see below. > > >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1074 > >> + priv->touchEventSequence = ++webkitWebViewBaseGlobalTouchEventSequence; > > > > Ah, this is the current sequence then. Can we use this also to see if there's an emulation in progress? when it's > 0. > > Actually, we cant use the sequence to check if there is an emulation in > progress as it will always be > 0 after the first touch sequence happens > (begin/update/end) while the boolean is false when we receive a touch end. Do we really need the sequence number after end? Can't we just set it to 0 on end? > >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1096 > >> + gdk_event_put(reinterpret_cast<GdkEvent*>(touchEvent)); > > > > Do we really need to synthesize events? Can't we just create a NativeWebTouchEvent in every case and send it to the web process? I also wonder if the GTK+ gesture controller doesn't handle the GTK_TEST_TOUCHSCREEN variable, so we could send the normal events to the gesture controller instead. > > Actually that is what I was doing before. My initial impl was invoking the > webkitWebViewBaseTouchEvent callback directly instead of queueing the event > and it would consume the mouse events when the callback returned TRUE - in > which case things like text selection with mouse wouldnt work when running > the touch emulation. > The thing is that in this case we get different behaviours. The current > patch here doesnt consume the mouse events, so text selection, for instance > works in this case (not sure if desirable). > > I changed it as I thought we could potentially have issues when not queueing > the event and processing it with the gesture controller - although it worked > fine with the tests. > > So, please let me know which way do you prefer. I don't know, this is actually a debug feature just for testing, it's probably ok if some things don't work, but I would like to know what Carlos Garnacho thinks about this. > >> Source/WebKit2/UIProcess/gtk/GestureController.cpp:159 > >> + } > > > > Is this an existing issue or something needed for this patch? > > This is actually an issue I found when working on the kinetic scrolling > support (bug #155750 - the last patch there includes this fix as part of my > changes). I dont remember exactly what happened but iirc > gtk_gesture_get_last_event would return NULL causing a crash on handleTap, > meaning some other gesture handled the sequence (or it was cancelled...) > while the DragGesture received a drag-begin/end. > > I can get a backtrace and open a new bug if needed for this but it is quite > easy to reproduce the crash without this change - mix scrolling with > dragging elements a few times and it blows away. If this is unrelated, please, file a different bug report.