[Webkit-unassigned] [Bug 193919] [GTK] Implement back/forward touchpad gesture

[bugzilla-daemon at webkit.org]

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

--- Comment #93 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 361288
  --> https://bugs.webkit.org/attachment.cgi?id=361288
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361288&action=review

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:84
> +    bool allowBackForwardNavigationGestures { false };

I would use enable instead of allow for this setting.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1460
> +     * Enable or disable horizontal swipe gesture for back-forward navigation.

Why do we need a setting for this? Would other kind of gestures also need a setting? Should we make this more generic then?

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:870
> +    WebKit::ViewGestureController& controller = webkitWebViewBaseViewGestureController(webViewBase);

WebKit:: is not needed here.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1175
> +    WebKitWebViewBasePrivate* priv = webViewBase->priv;
> +    if (!priv->viewGestureController)
> +        priv->viewGestureController = std::make_unique<WebKit::ViewGestureController>(*priv->pageProxy);
> +    return *priv->viewGestureController;

gesture controller and dnd helper are created on demand because they are only created if a gesture or dnd event is received, but we are creating the view gesture controller unconditionally in webkitWebViewBaseScrollEvent() and some other places, so I would just create it in webkitWebViewBaseCreateWebPage and simply return it here.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1670
> +    WebPageProxy* proxy = webkitWebViewBase->priv->pageProxy.get();

s/proxy/page/

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1689
> +    RefPtr<ViewSnapshot> snapshot = ViewSnapshot::create(WTFMove(surface));
> +    return snapshot;

return ViewSnapshot::create(WTFMove(surface));

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1694
> +    if (webkitWebViewBase->priv->viewGestureController)

We don't need to null-check here if we create the view gesture controller in webkitWebViewBaseCreateWebPage. Should we check if isSwipeGestureEnabled instead?

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1700
> +    if (webkitWebViewBase->priv->viewGestureController)

Ditto.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1706
> +    if (webkitWebViewBase->priv->viewGestureController)

Ditto.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1712
> +    if (webkitWebViewBase->priv->viewGestureController)

Ditto.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1718
> +    if (webkitWebViewBase->priv->viewGestureController)

Ditto.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1724
> +    if (webkitWebViewBase->priv->viewGestureController)

Ditto.

> Source/WebKit/UIProcess/ViewGestureController.h:368
> +        float getProgress();

getProgress -> progress()
Make it const and I think it can be implemented here since it simply returns m_pogress

> Source/WebKit/UIProcess/ViewGestureController.h:369
> +        SwipeDirection getDirection() { return m_direction; }

getDirection() -> direction() and make it const

> Source/WebKit/UIProcess/ViewGestureController.h:390
> +        double m_velocity;

Initialize this.

> Source/WebKit/UIProcess/ViewGestureController.h:398
> +        float m_startProgress;
> +        float m_endProgress;
> +        bool m_cancelled;

And these

> Source/WebKit/UIProcess/ViewGestureController.h:406
> +    cairo_pattern_t* m_currentSwipeSnapshotPattern { nullptr };

Use RefPtr<cairo_pattern_t>

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:115
> +        GtkWidget* widget = m_viewGestureController.m_webPageProxy.viewWidget();

There's m_webPageProxy in ViewGestureController::SwipeProgressTracker

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:151
> +    if (
> +#if GTK_CHECK_VERSION(3, 20, 0)
> +    event->is_stop
> +#else
> +    !event->delta_x && !event->delta_y
> +#endif
> +    ) {

This is difficult to read, add a helper function isEventStop() or something like that and move the #if there. It can be used in scrollEventCanEndSwipe too.

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:165
> +    m_progress = std::min(std::max(m_progress, minProgress), maxProgress);

clampTo ?

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:194
> +    GtkWidget* widget = m_viewGestureController.m_webPageProxy.viewWidget();

m_webPageProxy

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:247
> +    ViewSnapshot* snapshot = targetItem->snapshot();
> +    if (snapshot) {

if (auto* snapshot = targetItem->snapshot()) {

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:250
> +        FloatSize viewSize = FloatSize(m_webPageProxy.viewSize());

FloatSize viewSize(m_webPageProxy.viewSize());

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:258
> +                m_currentSwipeSnapshotPattern = cairo_pattern_create_rgba(color.red(), color.green(), color.blue(), color.alpha());

Does this work? Cairo expects doubles between 0 and 1, but WebCore::Color uses integers between 0 and 255. You should use color.getRGBA passing doubles

> Source/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk.cpp:70
> +    return stride*height;

stride * height

> Source/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk.cpp:76
> +        return WebCore::IntSize();

return { };

> Source/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk.cpp:82
> +    return WebCore::IntSize(width, height);

return { width, height };

> Source/WebKit/UIProcess/gtk/WebMemoryPressureHandlerGtk.cpp:39
> +    if (disableMemoryPressureMonitor && !strcmp(disableMemoryPressureMonitor, "1"))
> +        return;

I think it should be the caller the one checking the env var before calling this.

> Source/WebKit/UIProcess/gtk/WebMemoryPressureHandlerGtk.cpp:48
> +    auto& memoryPressureHandler = MemoryPressureHandler::singleton();
> +    memoryPressureHandler.setLowMemoryHandler([] (Critical critical, Synchronous) {
> +        ViewSnapshotStore::singleton().discardSnapshotImages();
> +
> +        for (auto* processPool : WebProcessPool::allProcessPools())
> +            processPool->handleMemoryPressureWarning(critical);
> +    });
> +    memoryPressureHandler.install();

Maybe we could share all this with mac port too. If we ever need to add platform specific stuff we can add a platform method inside the lambda. We would also need to move the

> Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:80
> +
> +    installMemoryPressureHandler();

We could check the env var here once, storing its result in a static and then use it also in platformInitializeWebProcess below.

-- 
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/20190207/02846f66/attachment.html>