[Webkit-unassigned] [Bug 212324] [GTK4] Add support for touch events

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=212324

--- Comment #7 from Carlos Garnacho <carlosg at gnome.org> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210506/71e69261/attachment.htm>