Bug 212324

Created attachment 427570 [details] Patch, rewrite GTK gesture support to work similarly in GTK3/4 Here's a (somewhat big) patch that rewrites touch and touchpad gesture handling so it's able to work on both GTK3 and GTK4 with the minimal amount of version-specific code. I believed this is the path of least resistance, rather than introducing a larger maze of "#if USE(GTK4)".

Oh awesome! Even the navigation gesture is ported. I tried to port it at some point and gave up with how touch sequences work with GTK4. I see this indeed required much bigger changes than what I did.

Comment on attachment 427570 [details] Patch, rewrite GTK gesture support to work similarly in GTK3/4 View in context: https://bugs.webkit.org/attachment.cgi?id=427570&action=review > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:417 > + graphene_point_t translation = { static_cast<float>(swipingLayerOffset), 0 }; Can we use things like &GRAPHENE_POINT_INIT() in WebKit? Then it could be like `gtk_snapshot_translate(snapshot, &GRAPHENE_POINT_INIT (whatever, 0));` Also, any reason to use `static_cast<float>` here but `(float)` below?

There are things like not using Cairo to draw the old page and the drop shadow, but I can port those myself later.

Comment on attachment 427570 [details] Patch, rewrite GTK gesture support to work similarly in GTK3/4 View in context: https://bugs.webkit.org/attachment.cgi?id=427570&action=review Looks great, I'm adding just a few comments. Patch needs to be rebased to apply on current trunk. > Source/WebKit/ChangeLog:27 > + No new tests, existing tests should catch regressions. Unfortunately we no longer run touch event tests, because touch events are now enabled at runtime depending on the hardware. So, we will some need manual testing here. > Source/WebKit/PlatformGTK.cmake:39 > +list(APPEND WebKit_SOURCES > + UIProcess/ViewGestureController.cpp > > - UIProcess/gtk/ViewGestureControllerGtk.cpp > + UIProcess/gtk/ViewGestureControllerGtk.cpp I think these are here only because they're conditionally added to the buil, so they can be moved to SourceGTK.txt > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:756 > + auto* pageSnapshot = gtk_snapshot_new(); > + webViewBase->priv->acceleratedBackingStore->snapshot(pageSnapshot); Can we do this also depending on isShowingNavigationGestureSnapshot? or isn't it worth it? > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:759 > + auto* pageRenderNode = gtk_snapshot_free_to_node(pageSnapshot); > + if (pageRenderNode) { if (auto* pageRenderNode = gtk_snapshot_free_to_node(pageSnapshot)) > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:766 > + } Where's pageRenderNode freed? > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1497 > +static void appendTouchEvent(WebKitWebViewBase* webViewBase, Vector<WebPlatformTouchPoint>& touchPoints, GdkEvent* event, WebPlatformTouchPoint::TouchPointState state) Make this receive a GtkWidget* and we avoid two casts below > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1520 > + GdkEventType type = gdk_event_get_event_type(event); > + switch (type) { switch (gdk_event_get_event_type(event)) { > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1567 > + if (!priv->touchEvents.size()) if (priv->touchEvents.isEmpty()) > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1587 > - break; > + return GDK_EVENT_PROPAGATE; Do we need to chain up in gtk3? > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1843 > + auto* page = webkitWebViewBaseGetPage(webViewBase); > + ASSERT(page); Use priv->pageProxy directly here. > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1860 > + auto* page = webkitWebViewBaseGetPage(webViewBase); > + ASSERT(page); Ditto. > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1866 > + double pageScale = priv->initialZoomScale * scale; > + if (pageScale < 1.0) > + pageScale = 1.0; > + if (pageScale > 3.0) > + pageScale = 3.0; auto pageScale = clampTo<double>(priv->initialZoomScale * scale, 1, 3); > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1881 > + WebKitWebViewBasePrivate* priv = webViewBase->priv; > + priv->isLongPressed = true; webViewBase->priv->isLongPressed = true; > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1887 > + WebKitWebViewBasePrivate* priv = webViewBase->priv; > + priv->isLongPressed = false; Same here, we don't need the local variable just for a single line > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1890 > +static void webkitWebViewBaseTouchRelease(WebKitWebViewBase* webViewBase, int nPress, double x, double y, GtkGesture*gesture) GtkGesture*gesture -> GtkGesture* gesture > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1913 > + GdkModifierType modifiers; > +#if USE(GTK4) > + modifiers = gtk_event_controller_get_current_event_state(GTK_EVENT_CONTROLLER(gesture)); > +#else > + gtk_get_current_event_state(&modifiers); > +#endif Add gtk_event_controller_get_current_event_state() impl for GTK3 in Source/WebCore/platform/gtk/GtkVersioning.h and we don't need ifdefs here. > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1915 > + webkitWebViewBaseSynthesizeMouseEvent(webViewBase, MouseEventType::Motion, 0, 0, x, y, modifiers, nPress, "touch"); Use touchPointerEventType() instead of "touch" > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1944 > + if (!gtk_drag_check_threshold(GTK_WIDGET(webViewBase), 0, 0, (int)offsetX, (int)offsetY)) Use static_cast<int>() > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1971 > +#if USE(GTK4) > + eventTime = static_cast<int32_t>(gtk_event_controller_get_current_event_time(GTK_EVENT_CONTROLLER(gesture))); > +#else > + eventTime = static_cast<int32_t>(gtk_get_current_event_time()); > +#endif Add a an implementation of gtk_event_controller_get_current_event_time() for gtk3 too. > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2083 > + priv->touchGestureGroup = gtk_gesture_zoom_new(viewWidget); In gtk4 ownership is transferred to GtkWidget, but where is this freed in gtk3? > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2092 > + auto* gesture = gtk_gesture_long_press_new(viewWidget); Same for the gesture here.

Created attachment 427869 [details] Updated patch.

(In reply to Carlos Garcia Campos from comment #5) > Comment on attachment 427570 [details] > Patch, rewrite GTK gesture support to work similarly in GTK3/4 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427570&action=review > > Looks great, I'm adding just a few comments. Patch needs to be rebased to > apply on current trunk. > > > Source/WebKit/ChangeLog:27 > > + No new tests, existing tests should catch regressions. > > Unfortunately we no longer run touch event tests, because touch events are > now enabled at runtime depending on the hardware. So, we will some need > manual testing here. Oh, unfortunate, reverted that to "oops". > > > Source/WebKit/PlatformGTK.cmake:39 > > +list(APPEND WebKit_SOURCES > > + UIProcess/ViewGestureController.cpp > > > > - UIProcess/gtk/ViewGestureControllerGtk.cpp > > + UIProcess/gtk/ViewGestureControllerGtk.cpp > > I think these are here only because they're conditionally added to the buil, > so they can be moved to SourceGTK.txt That worked :). > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:756 > > + auto* pageSnapshot = gtk_snapshot_new(); > > + webViewBase->priv->acceleratedBackingStore->snapshot(pageSnapshot); > > Can we do this also depending on isShowingNavigationGestureSnapshot? or > isn't it worth it? I left it as is so far. We create and throw away a GtkSnapshot this way, but the operation didn't look expensive, and the resulting render node tree is the same if we're not mid-gesture. > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:759 > > + auto* pageRenderNode = gtk_snapshot_free_to_node(pageSnapshot); > > + if (pageRenderNode) { > > if (auto* pageRenderNode = gtk_snapshot_free_to_node(pageSnapshot)) > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:766 > > + } > > Where's pageRenderNode freed? Oops, wrong kind of "auto" :), this is handled now. > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1497 > > +static void appendTouchEvent(WebKitWebViewBase* webViewBase, Vector<WebPlatformTouchPoint>& touchPoints, GdkEvent* event, WebPlatformTouchPoint::TouchPointState state) > > Make this receive a GtkWidget* and we avoid two casts below > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1520 > > + GdkEventType type = gdk_event_get_event_type(event); > > + switch (type) { > > switch (gdk_event_get_event_type(event)) { > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1567 > > + if (!priv->touchEvents.size()) > > if (priv->touchEvents.isEmpty()) > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1587 > > - break; > > + return GDK_EVENT_PROPAGATE; > > Do we need to chain up in gtk3? > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1843 > > + auto* page = webkitWebViewBaseGetPage(webViewBase); > > + ASSERT(page); > > Use priv->pageProxy directly here. > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1860 > > + auto* page = webkitWebViewBaseGetPage(webViewBase); > > + ASSERT(page); > > Ditto. > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1866 > > + double pageScale = priv->initialZoomScale * scale; > > + if (pageScale < 1.0) > > + pageScale = 1.0; > > + if (pageScale > 3.0) > > + pageScale = 3.0; > > auto pageScale = clampTo<double>(priv->initialZoomScale * scale, 1, 3); > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1881 > > + WebKitWebViewBasePrivate* priv = webViewBase->priv; > > + priv->isLongPressed = true; > > webViewBase->priv->isLongPressed = true; > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1887 > > + WebKitWebViewBasePrivate* priv = webViewBase->priv; > > + priv->isLongPressed = false; > > Same here, we don't need the local variable just for a single line > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1890 > > +static void webkitWebViewBaseTouchRelease(WebKitWebViewBase* webViewBase, int nPress, double x, double y, GtkGesture*gesture) > > GtkGesture*gesture -> GtkGesture* gesture > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1913 > > + GdkModifierType modifiers; > > +#if USE(GTK4) > > + modifiers = gtk_event_controller_get_current_event_state(GTK_EVENT_CONTROLLER(gesture)); > > +#else > > + gtk_get_current_event_state(&modifiers); > > +#endif > > Add gtk_event_controller_get_current_event_state() impl for GTK3 in > Source/WebCore/platform/gtk/GtkVersioning.h and we don't need ifdefs here. > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1915 > > + webkitWebViewBaseSynthesizeMouseEvent(webViewBase, MouseEventType::Motion, 0, 0, x, y, modifiers, nPress, "touch"); > > Use touchPointerEventType() instead of "touch" > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1944 > > + if (!gtk_drag_check_threshold(GTK_WIDGET(webViewBase), 0, 0, (int)offsetX, (int)offsetY)) > > Use static_cast<int>() > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1971 > > +#if USE(GTK4) > > + eventTime = static_cast<int32_t>(gtk_event_controller_get_current_event_time(GTK_EVENT_CONTROLLER(gesture))); > > +#else > > + eventTime = static_cast<int32_t>(gtk_get_current_event_time()); > > +#endif > > Add a an implementation of gtk_event_controller_get_current_event_time() for > gtk3 too. Did all these. > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2083 > > + priv->touchGestureGroup = gtk_gesture_zoom_new(viewWidget); > > In gtk4 ownership is transferred to GtkWidget, but where is this freed in > gtk3? > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2092 > > + auto* gesture = gtk_gesture_long_press_new(viewWidget); > > Same for the gesture here. Ah good point, seems it was leaked previously too. I made all these gestures dispose with the view via weak refs on GTK3.

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

Created attachment 427876 [details] Updated patch.

Comment on attachment 427876 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=427876&action=review > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1593 > - break; > + return GDK_EVENT_PROPAGATE; Do we need to chain up in gtk3? > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1954 > +#if USE(GTK4) > + eventTime = static_cast<int32_t>(gtk_event_controller_get_current_event_time(GTK_EVENT_CONTROLLER(gesture))); > +#else > + eventTime = static_cast<int32_t>(gtk_get_current_event_time()); > +#endif We can remove the ifdef here now > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2065 > + g_object_weak_ref(G_OBJECT(viewWidget), gestureWeakNotify, priv->touchGestureGroup); Since we are ignoring the oldLocation parameter, could we use g_object_unref here directly? You probably need a reinterpret_cast. Other solution would be to use g_object_set_data_full

(In reply to Carlos Garcia Campos from comment #10) > Comment on attachment 427876 [details] > Updated patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427876&action=review > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1593 > > - break; > > + return GDK_EVENT_PROPAGATE; > > Do we need to chain up in gtk3? Oh, missed replying to that in the previous round. In GTK3 this is the handler of the touch_event vmethod in GtkWidgetClass, it should always receive touch events and it shouldn't ever fall through this case. Perhaps it should assert there? > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1954 > > +#if USE(GTK4) > > + eventTime = static_cast<int32_t>(gtk_event_controller_get_current_event_time(GTK_EVENT_CONTROLLER(gesture))); > > +#else > > + eventTime = static_cast<int32_t>(gtk_get_current_event_time()); > > +#endif > > We can remove the ifdef here now Oops, true. > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2065 > > + g_object_weak_ref(G_OBJECT(viewWidget), gestureWeakNotify, priv->touchGestureGroup); > > Since we are ignoring the oldLocation parameter, could we use g_object_unref > here directly? You probably need a reinterpret_cast. Other solution would be > to use g_object_set_data_full Sure, I passed g_object_unref first but wondered about function casts :). Let me try that.

Comment on attachment 427876 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=427876&action=review >>> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1593 >>> + return GDK_EVENT_PROPAGATE; >> >> Do we need to chain up in gtk3? > > Oh, missed replying to that in the previous round. In GTK3 this is the handler of the touch_event vmethod in GtkWidgetClass, it should always receive touch events and it shouldn't ever fall through this case. Perhaps it should assert there? Yes, add ASSERT_NOT_REACHED();

Created attachment 427984 [details] [fast-cq] Updated patch. reinterpret_cast still triggered -Wcast-function-type with GWeakNotify, so I went for g_object_set_data_full() instead. This now does the assert and removed the unnecessary #if switch.

Comment on attachment 427984 [details] [fast-cq] Updated patch. This looks good to me. Alexander, can you confirm everything is working for you?

I don't have a build right now, I can just file issues later instead.

Committed r277172 (237458@main): <https://commits.webkit.org/237458@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427984 [details].

*** Bug 212325 has been marked as a duplicate of this bug. ***