[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 #5 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 427570
  --> https://bugs.webkit.org/attachment.cgi?id=427570
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.

-- 
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/20210504/144c6f83/attachment.htm>