[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 #6 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 360363
  --> https://bugs.webkit.org/attachment.cgi?id=360363
Patch

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

I was hoping it could be shared with WPE, but I see the bits in PageClientImpl and WebKitWebViewBase are really GTK-specific. OK then. Carlos Garcia will surely want to review it. And I suppose it's blocked on either bug #193903 or bug #193972. But it looks generally excellent to me.

My main concern is the massive code duplication in BackForwardGestureController.cpp and (possibly) ViewSnapshotStore.cpp. At the cost of making it harder to pass EWS and not break Mac, we should be able to share most of this code with the Cocoa classes you copied from, right? ViewGestureController.cpp is already C++, so that shouldn't be too hard to move to cross-platform and merge with BackForwardGestureController.cpp. Then in ViewSnapshotStore.mm... well, maybe there's enough differences there for that to remain platform-specific. I would start by trying to share as much of BackForwardGestureController and ViewGestureController as possible.

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

This needs a Since: 2.24 tag.  (Of course you can update it to 2.26 if it doesn't make it.)

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3544
> + * Returns: %TRUE If horizontal swipe gesture will trigger back-forward navigaiton or %FALSE otherwise.

if (lowercase i)

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3558
> + * @allowed: Value to be set

lowercase v

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:504
> +static void allowBackForwardNavigationGesturesChanged(WebKitSettings *settings, GParamSpec*, WebKitWebView* webView)

WebKitSettings* settings

> Source/WebKit/UIProcess/API/gtk/PageClientImpl.h:146
> -    void didRestoreScrollPosition() override { }
> +    void didRestoreScrollPosition() override;
>      void isPlayingAudioWillChange() final { }
>      void isPlayingAudioDidChange() final { }

I see this class is already a mix of override and final... you don't have to change this, since it's a preexisting problem and you matched the prevailing style (there were more "overrides" before), but understand of course the class should consistently use one or the other. (We argue about which, but "final" seems to be preferred in WebKit nowadays. There was a recent discussion on webkit-dev, but I don't remember how it ended.) This was a bit of a long review comment to tell you not to change anything, but I just wanted to flag this so you know it's not an example of good code.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1666
> +    cairo_t* cr = cairo_create(surface.get());

Surely this is leaked? Shouldn't it be:

RefPtr<cairo_t> cr = adoptRef(cairo_create(surface.get()));

> Source/WebKit/UIProcess/gtk/BackForwardGestureController.cpp:230
> +    RELEASE_LOG(ViewGestures, "Swipe Snapshot Removal (%0.2f ms) - %{public}s", sinceStart.milliseconds(), log.utf8().data());

Isn't this going to result in a -Wformat warning? %{public} is Mac-specific.

-- 
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/20190130/27593883/attachment.html>