Bug 193919 - [GTK] Implement back/forward touchpad gesture
Summary: [GTK] Implement back/forward touchpad gesture
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-28 12:35 PST by Alexander Mikhaylenko
Modified: 2019-02-09 02:37 PST (History)
15 users (show)

See Also:

Attachments
Patch (87.02 KB, patch)
2019-01-28 13:02 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-highsierra (2.02 MB, application/zip)
2019-01-28 14:49 PST, Build Bot 		no flags Details
Patch (134.11 KB, patch)
2019-01-31 07:05 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (134.39 KB, patch)
2019-01-31 07:12 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (135.89 KB, patch)
2019-01-31 07:54 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (135.96 KB, patch)
2019-01-31 08:19 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (157.46 KB, patch)
2019-01-31 11:11 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (162.07 KB, patch)
2019-01-31 12:22 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (162.03 KB, patch)
2019-01-31 12:38 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (162.03 KB, patch)
2019-01-31 13:22 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (162.04 KB, patch)
2019-01-31 13:33 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (162.05 KB, patch)
2019-01-31 13:55 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (163.60 KB, patch)
2019-01-31 14:15 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (163.59 KB, patch)
2019-01-31 14:35 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (163.59 KB, patch)
2019-01-31 14:51 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (164.49 KB, patch)
2019-01-31 15:32 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (166.59 KB, patch)
2019-01-31 16:01 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (163.65 KB, patch)
2019-02-01 00:16 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (165.51 KB, patch)
2019-02-01 00:54 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (165.58 KB, patch)
2019-02-01 04:27 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (98.71 KB, patch)
2019-02-03 13:12 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (98.61 KB, patch)
2019-02-03 13:17 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (165.42 KB, patch)
2019-02-03 13:46 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (165.37 KB, patch)
2019-02-04 03:09 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (171.18 KB, patch)
2019-02-04 08:00 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (171.10 KB, patch)
2019-02-04 10:53 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (12.78 MB, application/zip)
2019-02-04 12:42 PST, Build Bot 		no flags Details
Patch (193.60 KB, patch)
2019-02-05 05:53 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (193.65 KB, patch)
2019-02-05 06:15 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (193.91 KB, patch)
2019-02-05 06:46 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (194.78 KB, patch)
2019-02-05 06:57 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.70 MB, application/zip)
2019-02-05 08:15 PST, Build Bot 		no flags Details
Patch (194.84 KB, patch)
2019-02-06 04:32 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (193.16 KB, patch)
2019-02-06 04:47 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (205.37 KB, patch)
2019-02-08 09:59 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (205.79 KB, patch)
2019-02-08 11:44 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (208.36 KB, patch)
2019-02-08 12:16 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (208.35 KB, patch)
2019-02-08 12:47 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (207.72 KB, patch)
2019-02-08 13:00 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Patch (205.15 KB, patch)
2019-02-08 13:13 PST, Alexander Mikhaylenko 		no flags Details | Formatted Diff | Diff
Show Obsolete (39) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Mikhaylenko 2019-01-28 12:35:05 PST 
This is a very nice-to-have feature, and the existing Cocoa/Mac implementation can be ported relatively simply. Patch incoming.
Comment 1 Alexander Mikhaylenko 2019-01-28 13:02:03 PST 
Created attachment 360363 [details]
Patch
Comment 2 Build Bot 2019-01-28 13:05:06 PST 
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Alexander Mikhaylenko 2019-01-28 13:23:08 PST 
This is also currently broken with AcceleratedBackingStoreWayland, see bug 193903
Comment 4 Build Bot 2019-01-28 14:49:30 PST Comment hidden (obsolete)
Comment 5 Build Bot 2019-01-28 14:49:32 PST Comment hidden (obsolete)
Comment 6 Michael Catanzaro 2019-01-29 16:36:17 PST 
Comment on attachment 360363 [details]
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.
Comment 7 Alexander Mikhaylenko 2019-01-30 02:55:20 PST 
(In reply to Michael Catanzaro from comment #6)
> 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.
Yes, it relies on GTK heavily for snapshotting and event handling.

> 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.
Agree, but I have a few questions about implementation that are probably easier discussed in IRC.

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

> > 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)
Oops. Also, looking at it now, isn't "if ... will" a grammatical error?

> lowercase v
Oops.

> WebKitSettings* settings
Huh, I assumed the style checking script caught all of these :o

> 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.
Ok.

> Surely this is leaked? Shouldn't it be:
> 
> RefPtr<cairo_t> cr = adoptRef(cairo_create(surface.get()));
Oops, it's definitely leaked. Though I get an error with cairo_surface_destroy() in ViewSnapshotStore.cpp being called on already destroyed surface if I do this. Is that one not necessary? I admit I don't completely understand when it gets ref-d/unref-d.

> > 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.
Ouch, there was actually a warning there that I was meaning to fix, but forgot. :x
Though it's a different warning, and I don't completely understand where it's coming from:
> /home/exalm/Projects/WebKit/Source/WebKit/UIProcess/gtk/BackForwardGestureController.cpp:229:10: warning: variable ‘sinceStart’ set but not used [-Wunused-but-set-variable]
>     auto sinceStart = MonotonicTime::now() - m_startTime;

I assume RELEASE_LOG() is a macro is substituted with nothing, which would also explain a lack of format warning. What is the preferred way of logging in gtk parts?
Comment 8 Alexander Mikhaylenko 2019-01-30 07:37:52 PST 
Just for reference, the patch from  bug 193903 fixes the gesture as well :)
Comment 9 Michael Catanzaro 2019-01-30 11:09:01 PST 
(In reply to Alexander Mikhaylenko from comment #7)
> Oops. Also, looking at it now, isn't "if ... will" a grammatical error?

Nope. Why do you think it would be?

I'd put a comma before ", or %FALSE otherwise."

> > WebKitSettings* settings
> Huh, I assumed the style checking script caught all of these :o

It normally does; not sure why it missed this one.

(In reply to Alexander Mikhaylenko from comment #7)
> Oops, it's definitely leaked. Though I get an error with
> cairo_surface_destroy() in ViewSnapshotStore.cpp being called on already
> destroyed surface if I do this. Is that one not necessary? I admit I don't
> completely understand when it gets ref-d/unref-d.

Without investigating the actual refcounting issue here: the RefPtr/GRefPtr is basically "safer g_autoptr" where unref occurs automatically at the bottom of the scope. The main difference is that you have to adopt the initial ref with adoptRef/adoptGRef, otherwise you wind up with an initial refcount of 2.

> Though it's a different warning, and I don't completely understand where
> it's coming from:
> > /home/exalm/Projects/WebKit/Source/WebKit/UIProcess/gtk/BackForwardGestureController.cpp:229:10: warning: variable ‘sinceStart’ set but not used [-Wunused-but-set-variable]
> >     auto sinceStart = MonotonicTime::now() - m_startTime;
> 
> I assume RELEASE_LOG() is a macro is substituted with nothing, which would
> also explain a lack of format warning. What is the preferred way of logging
> in gtk parts?

Hm, you might be building with logging disabled? I think logging is automatically enabled in debug builds, but I'm surprised the release log would not be enabled in release builds. Anyway, the RELEASE_LOG(foo) should ultimately boil down to a fprintf(stderr, foo), but it takes me half an hour every time I investigate all the various layers involved.

Maybe you could do something like

#if !RELEASE_LOG_DISABLED
    auto sinceStart = ...;
    RELEASE_LOG(...);
#endif
Comment 10 Adrian Perez 2019-01-30 11:51:07 PST 
Just a quick tip, without having the intention of hijacking the review...

(In reply to Michael Catanzaro from comment #9)
> (In reply to Alexander Mikhaylenko from comment #7)
>
> > Oops, it's definitely leaked. Though I get an error with
> > cairo_surface_destroy() in ViewSnapshotStore.cpp being called on already
> > destroyed surface if I do this. Is that one not necessary? I admit I don't
> > completely understand when it gets ref-d/unref-d.
> 
> Without investigating the actual refcounting issue here: the RefPtr/GRefPtr
> is basically "safer g_autoptr" where unref occurs automatically at the
> bottom of the scope. The main difference is that you have to adopt the
> initial ref with adoptRef/adoptGRef, otherwise you wind up with an initial
> refcount of 2.

This wiki page is a “hidden” a few clicks away from the front page,
and when I found it for the first time, it turned out to be a good
read to understand GRefPtr -- it even comes with examples:

  https://trac.webkit.org/wiki/GRefPtr

I hope this helps :)
Comment 11 Alexander Mikhaylenko 2019-01-31 07:05:25 PST 
Created attachment 360724 [details]
Patch
Comment 12 Alexander Mikhaylenko 2019-01-31 07:06:38 PST 
It's not finished, I haven't fixed cairo_t leak yet, and ViewSnapshotStore can most probably be shared as well.
Comment 13 Alexander Mikhaylenko 2019-01-31 07:12:10 PST 
Created attachment 360725 [details]
Patch
Comment 14 Build Bot 2019-01-31 07:14:09 PST Comment hidden (obsolete)
Comment 15 Alexander Mikhaylenko 2019-01-31 07:54:13 PST 
Created attachment 360726 [details]
Patch
Comment 16 Build Bot 2019-01-31 07:55:40 PST Comment hidden (obsolete)
Comment 17 Alexander Mikhaylenko 2019-01-31 08:19:26 PST 
Created attachment 360728 [details]
Patch
Comment 18 Build Bot 2019-01-31 08:21:24 PST Comment hidden (obsolete)
Comment 19 Alexander Mikhaylenko 2019-01-31 11:11:46 PST 
Created attachment 360745 [details]
Patch
Comment 20 Build Bot 2019-01-31 11:15:37 PST Comment hidden (obsolete)
Comment 21 Alexander Mikhaylenko 2019-01-31 12:22:27 PST 
Created attachment 360753 [details]
Patch
Comment 22 Build Bot 2019-01-31 12:26:09 PST Comment hidden (obsolete)
Comment 23 Alexander Mikhaylenko 2019-01-31 12:38:12 PST 
Created attachment 360755 [details]
Patch
Comment 24 Build Bot 2019-01-31 12:41:47 PST Comment hidden (obsolete)
Comment 25 Alexander Mikhaylenko 2019-01-31 13:22:47 PST 
Created attachment 360763 [details]
Patch
Comment 26 Build Bot 2019-01-31 13:25:42 PST Comment hidden (obsolete)
Comment 27 Alexander Mikhaylenko 2019-01-31 13:33:06 PST 
Created attachment 360765 [details]
Patch
Comment 28 Build Bot 2019-01-31 13:35:03 PST Comment hidden (obsolete)
Comment 29 Alexander Mikhaylenko 2019-01-31 13:55:19 PST 
Created attachment 360770 [details]
Patch
Comment 30 Build Bot 2019-01-31 13:57:57 PST Comment hidden (obsolete)
Comment 31 Alexander Mikhaylenko 2019-01-31 14:15:05 PST 
Created attachment 360780 [details]
Patch
Comment 32 Build Bot 2019-01-31 14:19:20 PST Comment hidden (obsolete)
Comment 33 Alexander Mikhaylenko 2019-01-31 14:35:12 PST 
Created attachment 360785 [details]
Patch
Comment 34 Build Bot 2019-01-31 14:38:12 PST Comment hidden (obsolete)
Comment 35 Alexander Mikhaylenko 2019-01-31 14:51:57 PST 
Created attachment 360789 [details]
Patch
Comment 36 Build Bot 2019-01-31 14:54:51 PST Comment hidden (obsolete)
Comment 37 Alexander Mikhaylenko 2019-01-31 15:32:47 PST 
Created attachment 360795 [details]
Patch
Comment 38 Build Bot 2019-01-31 15:36:15 PST Comment hidden (obsolete)
Comment 39 Alexander Mikhaylenko 2019-01-31 16:01:59 PST 
Created attachment 360800 [details]
Patch
Comment 40 Build Bot 2019-01-31 16:06:06 PST Comment hidden (obsolete)
Comment 41 Michael Catanzaro 2019-01-31 17:47:26 PST 
Comment on attachment 360800 [details]
Patch

Careful, you've duplicated your changelog in Tools/ChangeLog
Comment 42 Alexander Mikhaylenko 2019-02-01 00:16:21 PST 
Created attachment 360846 [details]
Patch
Comment 43 Build Bot 2019-02-01 00:18:17 PST Comment hidden (obsolete)
Comment 44 Alexander Mikhaylenko 2019-02-01 00:23:11 PST 
> Careful, you've duplicated your changelog in Tools/ChangeLog

Oops. I admit I'm not checking the changelog until it builds. :/
Comment 45 Alexander Mikhaylenko 2019-02-01 00:54:02 PST 
Created attachment 360848 [details]
Patch
Comment 46 Build Bot 2019-02-01 00:56:23 PST Comment hidden (obsolete)
Comment 47 Alexander Mikhaylenko 2019-02-01 04:27:06 PST 
Created attachment 360858 [details]
Patch
Comment 48 Alexander Mikhaylenko 2019-02-01 04:28:21 PST 
Ok, this one should be good (aside from the code style script needing an update)
Comment 49 Build Bot 2019-02-01 04:30:13 PST Comment hidden (obsolete)
Comment 50 Michael Catanzaro 2019-02-02 15:01:15 PST 
Alex, who'd be a good Apple reviewer for this? This is another big patch like we have in bug #174816.
Comment 51 Alexander Mikhaylenko 2019-02-03 13:12:02 PST 
Created attachment 361024 [details]
Patch
Comment 52 Alexander Mikhaylenko 2019-02-03 13:14:26 PST 
I've rebased the patch and removed Wayland check. There's a bug where we don't get any events when user lifts fingers from touchpad (so the gesture becomes stuck mid-progress); I assumed it always happens on X11, but turns out it only happens in XWayland in a Wayland session.
Comment 53 Alexander Mikhaylenko 2019-02-03 13:17:13 PST 
Created attachment 361026 [details]
Patch
Comment 54 Alexander Mikhaylenko 2019-02-03 13:18:20 PST 
Sorry for the churn, forgot to update changelog for the Wayland change :x
Comment 55 Michael Catanzaro 2019-02-03 13:24:03 PST 
Comment on attachment 360858 [details]
Patch

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

Really good work getting this to build and pass tests for Mac without a Mac. Hopefully the gesture still works there. Would be amazing if someone from Apple could check; otherwise, I'm sure it will be noticed quickly if it breaks.

This passes my review, but it needs to be approved by an owner too. I assume the goal is for there to be no functional changes on Apple platforms, right? That should make reviewing much easier.

> Source/WebKit/ChangeLog:21
> +        This is only enabled on Wayland, since on X11 we don't get any events when
> +        the gesture ends. Also, it's only allowed for touchpads, even though it can
> +        theoretically work with touch mice and trackpoints.

I know you discovered it doesn't need to be limited to Wayland; will you update this patch again?

> Source/WebKit/SourcesCocoa.txt:207
> +UIProcess/ViewGestureController.cpp
> +UIProcess/ViewSnapshotStore.cpp @no-unify

And you'll report a new bug to un-unify it?

> Source/WebKit/SourcesGTK.txt:116
> +UIProcess/ViewGestureController.cpp @no-unify
> +UIProcess/ViewSnapshotStore.cpp

For SourcesCocoa.txt you build ViewGestureController.cpp in unified build and ViewSnapshotStore.cpp with @no-unify; here you do the opposite. Let's match what you've done for Cocoa?

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:85
> +#if PLATFORM(GTK)
> +    bool allowBackForwardNavigationGestures { false };
> +#endif

Any reason you chose for it to be disabled by default? It means very few apps will enable it.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:510
> +    ViewGestureController* controller = &webkitWebViewBaseViewGestureController(WEBKIT_WEB_VIEW_BASE(webView));
> +
> +    if (controller)
> +        controller->setSwipeGestureEnabled(allowsBackForwardNavigationGestures);

webkitWebViewBaseViewGestureController returns a reference, which is guaranteed to be non-null (or it's a programmer error if it somehow is), so converting it to a pointer and then checking if it's null doesn't make sense. Just assume it's nonnull and use it. If that's ever wrong, you have a bug elsewhere.

> Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:71
> +    WebPageProxy* pageProxy = webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(m_viewWidget));

Do we need to null-check pageProxy? If not, add an ASSERT(pageProxy), please. Since webkitWebViewBaseGetPage returns a pointer and not a reference, we should assume it can be nullptr unless proven otherwise.

I almost asked you to do this in a bunch of places in WebKitWebViewBase.cpp as well, but that file has a pervasive assumption that it is nonnull. Shame it doesn't look easy to convert from RefPtr to Ref (like RefPtr, but can't be nullptr).

> Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:424
> +    ViewGestureController& controller = webkitWebViewBaseViewGestureController(WEBKIT_WEB_VIEW_BASE(m_viewWidget));
> +
> +    if (controller.isSwipeGestureEnabled()) {

Style nit: we'd normally avoid the blank line here, since grabbing controller is just setup for the conditional.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:206
>  #if HAVE(GTK_GESTURES)
>      std::unique_ptr<GestureController> gestureController;
>  #endif
> +    std::unique_ptr<ViewGestureController> viewGestureController;

I guess it doesn't use GTK+ gestures?

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:554
> +        cairo_pattern_t* group = cairo_pop_group(cr);
> +        controller.draw(cr, group);
> +        cairo_pattern_destroy(group);

Can you use a RefPtr here? We try to avoid holding ownership in raw pointers whenever possible.

#include <WebCore/RefPtrCairo.h>

RefPtr<cairo_pattern_t> group = adoptRef(cairo_pop_group(cr));
controller.draw(cr, group.get());

Even if you were about transfer ownership immediately thereafter, we like to adopt it in a RefPtr and then call leakRef() to make the transfer explicit.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1679
> +    return WTFMove(snapshot);

Never move into a return statement: it breaks return value optimization. And the function returns a RefPtr<WebKit::ViewSnapshot>, not a RefPtr<WebKit::ViewSnapshot>&&. Better simply:

return snapshot;

You can also remove the extra blank lines here and above. We don't normally leave a blank line between declaring a variable and using it.

> Source/WebKit/UIProcess/ViewGestureController.cpp:161
> +    if (auto loadCallback = WTFMove(m_loadCallback))
> +        loadCallback();

I know this is a problem with the existing code, but this is no good. We should be explicit in resetting m_loadCallback, rather than just trusting the move constructor will reset it to nullptr for us. (Which I'm sure it does, otherwise this would not work, but it's just good practice.)

if (auto loadCallback = std::exchange(m_loadCallback, nullptr))
    loadCallback();

> Source/WebKit/UIProcess/ViewGestureController.h:258
> +    CALayer *determineSnapshotLayerParent() const;
> +    CALayer *determineLayerAdjacentToSnapshotForParent(SwipeDirection, CALayer *snapshotLayerParent) const;

CALayer*

> Source/WebKit/UIProcess/ViewGestureController.h:346
> +    UIView *m_liveSwipeView { nullptr };

UIView*

> Source/WebKit/UIProcess/ViewSnapshotStore.cpp:53
> +ViewSnapshotStore& ViewSnapshotStore::singleton()
> +{
> +    static ViewSnapshotStore& store = *new ViewSnapshotStore;
> +    return store;

Go ahead and change it to use NeverDestroyed<ViewSnapshotStore> here.

Of course, better to not have singletons at all, but that's a preexisting problem and this isn't the right patch to try solving it.

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:66
> +    GdkDevice* device = gdk_event_get_source_device((GdkEvent*) event);

No C-style casts allowed!

static_cast<GdkEvent*>(event);

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:201
> +    m_swipeProgressTracker.startTracking(targetItem, direction);

WTFMove(targetItem),

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:218
> +void ViewGestureController::SwipeProgressTracker::startTracking(RefPtr<WebBackForwardListItem> targetItem, SwipeDirection direction)

RefPtr<WebBackForwardListItem>&& targetItem,

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:220
> +    if (m_state  != State::None)

Extra space here

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:250
> +bool ViewGestureController::SwipeProgressTracker::handleEvent(GdkEventScroll *event)

GdkEventScroll* event

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:363
> +void ViewGestureController::beginSwipeGesture(WebBackForwardListItem* targetItem, SwipeDirection direction)
> +{
> +    m_webPageProxy.navigationGestureDidBegin();
> +
> +    willBeginGesture(ViewGestureType::Swipe);
> +
> +    ViewSnapshot* snapshot = targetItem->snapshot();

Since you're assuming it's nonnull, this function should take a WebBackForwardListItem&, not a WebBackForwardListItem*. Or if it can be nullptr, better null-check it. Or, if it needs to be a pointer to match Apple's implementation, you can ASSERT(targetItem).

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:386
> +void ViewGestureController::handleSwipeGesture(WebBackForwardListItem* targetItem, double progress, SwipeDirection direction)
> +{
> +    gtk_widget_queue_draw(m_webPageProxy.viewWidget());
> +}

I guess we have unused variable warnings turned off for Source/WebKit, but you should still omit the parameter names if they're not used.

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:397
> +void ViewGestureController::endSwipeGesture(WebBackForwardListItem* targetItem, bool cancelled)
> +{
> +    if (cancelled) {
> +        removeSwipeSnapshot();
> +        m_webPageProxy.navigationGestureDidEnd(false, *targetItem);

Again: since you're assuming it's nonnull, this function should take a WebBackForwardListItem&, not a WebBackForwardListItem*. Or if it can be nullptr, better null-check it. Or, if it needs to be a pointer to match Apple's implementation, you can ASSERT(targetItem).

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

if (ViewSnapshot* snapshot = targetItem->snapshot())
    ...;

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:454
> +void ViewGestureController::draw(cairo_t* cr, cairo_pattern_t* pageGroup)

This is one of our few exceptions to our rule to use references rather than pointers. We actually do use pointers for GObjects and other C API types. I don't think it makes sense, since it gives up the safety of references, but anyway, that's what we do. So no need to change anything here.

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:473
> +    cairo_pattern_t* shadowPattern = cairo_pattern_create_linear(0, 0, -swipeOverlayShadowWidth, 0);

RefPtr<cairo_pattern_t> shadowPattern = adoptRef(cairo_pattern_create_linear(...));

Then you change direct use of shadowPattern to shadowPattern.get(), and kill the call to cairo_pattern_destroy.

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:537
> +bool ViewGestureController::beginSimulatedSwipeInDirectionForTesting(SwipeDirection)
> +{
> +    notImplemented();
> +    return false;
> +}
> +
> +bool ViewGestureController::completeSimulatedSwipeInDirectionForTesting(SwipeDirection)
> +{
> +    notImplemented();
> +    return false;
> +}

Don't actually call notImplemented() unless it's a TODO to implement it in the future. It creates a spammy debug warning. Better just return false.

> Source/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk.cpp:36
> +Ref<ViewSnapshot> ViewSnapshot::create(RefPtr<cairo_surface_t> surface)
> +{
> +    return adoptRef(*new ViewSnapshot(WTFMove(surface)));

RefPtr<cairo_surface_t>&& surface

> Source/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk.cpp:40
> +ViewSnapshot::ViewSnapshot(RefPtr<cairo_surface_t> surface)
> +    : m_surface(WTFMove(surface))

RefPtr<cairo_surface_t>&& surface
Comment 56 Michael Catanzaro 2019-02-03 13:24:30 PST 
Comment on attachment 360858 [details]
Patch

I actually reviewed the older version of your patch. I'll assume you didn't make it worse. ;)
Comment 57 Alexander Mikhaylenko 2019-02-03 13:46:46 PST 
Created attachment 361027 [details]
Patch
Comment 58 Build Bot 2019-02-03 13:50:02 PST Comment hidden (obsolete)
Comment 59 Alexander Mikhaylenko 2019-02-03 14:04:43 PST 
The last patch doesn't address the comments from comment 55, it's a fix for the last 2 borked patches where I forgot to add new files before uploading.

---

> I know you discovered it doesn't need to be limited to Wayland; will you update this patch again?

Yes, already done in the later versions.

> And you'll report a new bug to un-unify it?

Ok, when it's in :)

> For SourcesCocoa.txt you build ViewGestureController.cpp in unified build and ViewSnapshotStore.cpp with @no-unify; here you do the opposite. Let's match what you've done for Cocoa?

Sure. That was an attempt to get it to build somehow :D

> Any reason you chose for it to be disabled by default? It means very few apps will enable it.

It's disabled by default on Mac, and also this way we don't change behavior for the existing apps, like in auth widgets, like the Firefox Sync one in Epiphany preferences.

I can enable it by default if you'd prefer that.

> I guess it doesn't use GTK+ gestures?

It doesn't, it uses scroll events. That's the only reason it works in X11.

> Don't actually call notImplemented() unless it's a TODO to implement it in the future. It creates a spammy debug warning. Better just return false.

This was in the equivalent Mac code, but ok.

The rest of comments: ok
Comment 60 Alexander Mikhaylenko 2019-02-04 03:09:25 PST 
Created attachment 361050 [details]
Patch
Comment 61 Build Bot 2019-02-04 03:12:27 PST Comment hidden (obsolete)
Comment 62 Michael Catanzaro 2019-02-04 06:52:24 PST 
r=me, but it needs two more reviews:

 * Since it adds new GTK API, it needs to be approved by another GTK reviewer. Normally that would be Carlos Garcia.
 * Since it modifies cross-platform and Apple code under Source/WebKit, it needs to be approved by an owner
Comment 63 Alexander Mikhaylenko 2019-02-04 08:00:19 PST 
Created attachment 361056 [details]
Patch
Comment 64 Build Bot 2019-02-04 08:04:25 PST Comment hidden (obsolete)
Comment 65 Tim Horton 2019-02-04 09:37:05 PST 
(In reply to Michael Catanzaro from comment #62)
> r=me, but it needs two more reviews:
> 
>  * Since it adds new GTK API, it needs to be approved by another GTK
> reviewer. Normally that would be Carlos Garcia.
>  * Since it modifies cross-platform and Apple code under Source/WebKit, it
> needs to be approved by an owner

Also I wrote 90% of this in the first place so I’d love to look over it too :P Cc’ing myself to look in a bit
Comment 66 Alexander Mikhaylenko 2019-02-04 10:53:42 PST 
Created attachment 361074 [details]
Patch
Comment 67 Alexander Mikhaylenko 2019-02-04 10:56:36 PST 
Another reupload :)

Fixed a bug I've probably introduced during merging the changes back into Mac implementation. (At least I remember already doing this change before, should reset SwipeProgressTracker state after calling m_webPageProxy.navigationGestureSnapshotWasRemoved(), not before, otherwise there's a rare flickering)
Comment 68 Build Bot 2019-02-04 10:57:25 PST Comment hidden (obsolete)
Comment 69 Build Bot 2019-02-04 12:42:22 PST Comment hidden (obsolete)
Comment 70 Build Bot 2019-02-04 12:42:25 PST Comment hidden (obsolete)
Comment 71 Michael Catanzaro 2019-02-04 12:46:11 PST 
Don't worry about the test failure. Pretty sure you didn't break WebRTC. Tests are flaky sometimes.
Comment 72 Tim Horton 2019-02-04 13:03:46 PST 
Comment on attachment 361074 [details]
Patch

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

> Source/WebKit/Shared/SessionState.h:28
> +#if PLATFORM(COCOA) || PLATFORM(GTK)

Now that it's not in /Cocoa you can just include it unconditionally

> Source/WebKit/UIProcess/ViewGestureController.h:32
> +#if PLATFORM(GTK)
> +#include <WebCore/CairoUtilities.h>
> +#endif

Conditional includes should be in their own paragraph down below

> Source/WebKit/UIProcess/ViewGestureController.h:170
> +    void draw(cairo_t*, cairo_pattern_t*);

Probably stick this before the `// Testing` section

> Source/WebKit/UIProcess/ViewSnapshotStore.h:69
> +#else

Shouldn't most of these #else's be #elif PLATFORM(GTK) to make it easier to add more platforms later?

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:46
> +static const double swipeOverlayShadowGradientOffsets[] = { 0, 0.03125, 0.0625, 0.0938, 0.125, 0.1875, 0.25, 0.375, 0.4375, 0.5, 0.5625, 0.625, 0.6875, 0.75, 0.875, 1. };
> +static const double swipeOverlayShadowGradientAlpha[] = { 1, 0.99, 0.98, 0.95, 0.92, 0.82, 0.71, 0.46, 0.35, 0.25, 0.17, 0.11, 0.07, 0.04, 0.01, 0. };

Do you really want the modern-macOS-style gradients? That is surprising!

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:77
> +bool ViewGestureController::PendingSwipeTracker::scrollEventCanBecomeSwipe(GdkEventScroll *event, ViewGestureController::SwipeDirection& potentialSwipeDirection)

I wonder if we can factor some more of this out into shared code (a variant of this that just takes deltas, which scrollEventCanBecomeSwipe calls after extracting the deltas from the event, for example)

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:105
> +bool ViewGestureController::PendingSwipeTracker::handleEvent(GdkEventScroll *event)

Ditto
Comment 73 Alexander Mikhaylenko 2019-02-04 13:39:05 PST 
> Now that it's not in /Cocoa you can just include it unconditionally

Ok

> Shouldn't most of these #else's be #elif PLATFORM(GTK) to make it easier to add more platforms later?

It should.

> Do you really want the modern-macOS-style gradients? That is surprising!

Sorry :p Just ported what there was, and nobody minded, so I didn't change it. :)

> I wonder if we can factor some more of this out into shared code (a variant of this that just takes deltas, which scrollEventCanBecomeSwipe calls after extracting the deltas from the event, for example)

Hmm, yes. It also uses phase in macOS code, and is_stop in GTK code (and a device type check, but I should probably remove that one anyway), so should be doable. Not sure about handleEvent(), since it calls tryToStartSwipe(), which in turn calls trackSwipeEventWithOptions on macOS, so we still need the original event, but can probably factor out the checks.
Comment 74 Alexander Mikhaylenko 2019-02-05 05:53:13 PST 
Created attachment 361184 [details]
Patch
Comment 75 Build Bot 2019-02-05 05:56:36 PST Comment hidden (obsolete)
Comment 76 Alexander Mikhaylenko 2019-02-05 06:02:28 PST 
Addressed comments from comment 72.

I've moved pretty much all shared code (except beginSwipeGesture(), removeSwipeSnapshot() and platformTeardown(), there are just a couple shared lines there, and major differences otherwise) to ViewGestureController.cpp.

As said, the original events are still needed in some places, so I've added the following functions:

        bool scrollEventCanStartSwipe(PlatformScrollEvent);
        bool scrollEventCanEndSwipe(PlatformScrollEvent);
        WebCore::FloatSize scrollEventGetScrollingDeltas(PlatformScrollEvent);

that are implemented in Mac- and GTK-specific files, and made cross-platform logic uses these for accessing any properties and PlatformScrollEvent in signatures.

Also while merging them, noticed that render tree size threshold logic, that I initially assumed was Mac-specific, should be used in GTK as well, so that part is now also shared, and there are a few changes in WebProcess/Page/ (just re-shuffling `#if`s) to enable that.

As always, I don't know if the above patch builds on Mac, EWS will tell :)

> Do you really want the modern-macOS-style gradients? That is surprising!

I've asked GNOME designers about it, they said it looks fine :)
Comment 77 Alexander Mikhaylenko 2019-02-05 06:15:55 PST 
Created attachment 361185 [details]
Patch
Comment 78 Build Bot 2019-02-05 06:19:31 PST Comment hidden (obsolete)
Comment 79 Alexander Mikhaylenko 2019-02-05 06:46:43 PST 
Created attachment 361187 [details]
Patch
Comment 80 Build Bot 2019-02-05 06:50:53 PST Comment hidden (obsolete)
Comment 81 Alexander Mikhaylenko 2019-02-05 06:57:13 PST 
Created attachment 361188 [details]
Patch
Comment 82 Build Bot 2019-02-05 07:00:38 PST Comment hidden (obsolete)
Comment 83 Alexander Mikhaylenko 2019-02-05 07:35:02 PST 
Ok, the last one seems to build on all platforms.
Comment 84 Alexander Mikhaylenko 2019-02-05 07:57:06 PST 
Nevermind, it seems to actually break some tests rather than them being flaky.
Comment 85 Build Bot 2019-02-05 08:15:45 PST Comment hidden (obsolete)
Comment 86 Build Bot 2019-02-05 08:15:48 PST Comment hidden (obsolete)
Comment 87 Alexander Mikhaylenko 2019-02-06 04:32:28 PST 
Created attachment 361287 [details]
Patch
Comment 88 Alexander Mikhaylenko 2019-02-06 04:47:56 PST 
Created attachment 361288 [details]
Patch
Comment 89 Build Bot 2019-02-06 04:50:19 PST Comment hidden (obsolete)
Comment 90 Michael Catanzaro 2019-02-06 09:34:44 PST 
Two days past API freeze... we might try landing this anyway. :)

Tim, I assume you're fine with it unless you say otherwise, since you've reviewed it.

Still needs a second WPE/GTK API review.
Comment 91 Michael Catanzaro 2019-02-06 09:42:51 PST 
Comment on attachment 361288 [details]
Patch

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

> Source/WebKit/UIProcess/gtk/WebMemoryPressureHandlerGtk.cpp:37
> +    const char* disableMemoryPressureMonitor = getenv("WEBKIT_DISABLE_MEMORY_PRESSURE_MONITOR");

We'll need to add it to https://trac.webkit.org/wiki/EnvironmentVariables once this lands.

Also, if we add this envvar, we should surely use it for the WebCore-level memory pressure handler as well.

I've wanted WebKit-level memory pressure handler for a long time, by the way. This is cool!
Comment 92 Alexander Mikhaylenko 2019-02-06 10:30:21 PST 
That variable was already used for WebKitTestRunner: https://github.com/WebKit/webkit/search?q=WEBKIT_DISABLE_MEMORY_PRESSURE_MONITOR&unscoped_q=WEBKIT_DISABLE_MEMORY_PRESSURE_MONITOR
Comment 93 Carlos Garcia Campos 2019-02-07 01:00:01 PST 
Comment on attachment 361288 [details]
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.
Comment 94 Alexander Mikhaylenko 2019-02-07 04:12:39 PST 
> Why do we need a setting for this? Would other kind of gestures also need a setting? Should we make this more generic then?

It's only useful for browsers, but not e.g. for auth widgets. So I assume very few apps will actually want this, like Epiphany, Yelp, Devhelp in GNOME, but not auth widgets in control center, or RH login in Boxes, or Google Docs widget in Documents, or mail display in Geary, etc.

> 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

Ahh. I just assumed every page I tested had white background. :(

> 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

I wanted to, but the Cocoa one has this comment:

// FIXME: This should be able to share code with WebCore's MemoryPressureHandler (and be platform independent).
// Right now it cannot because WebKit1 and WebKit2 need to be able to coexist in the UI process,
// and you can only have one WebCore::MemoryPressureHandler.

We can though add it directly there specifically with #if PLATFORM(GTK) and drop the added memory pressure handler.

The rest: sure
Comment 95 Carlos Garcia Campos 2019-02-07 22:23:50 PST 
(In reply to Alexander Mikhaylenko from comment #94)
> > Why do we need a setting for this? Would other kind of gestures also need a setting? Should we make this more generic then?
> 
> It's only useful for browsers, but not e.g. for auth widgets. So I assume
> very few apps will actually want this, like Epiphany, Yelp, Devhelp in
> GNOME, but not auth widgets in control center, or RH login in Boxes, or
> Google Docs widget in Documents, or mail display in Geary, etc.

Would other kind of gestures also need a setting? Should we make this more generic then? I mean future gestures.

> > 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
> 
> Ahh. I just assumed every page I tested had white background. :(
> 
> > 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
> 
> I wanted to, but the Cocoa one has this comment:
> 
> // FIXME: This should be able to share code with WebCore's
> MemoryPressureHandler (and be platform independent).
> // Right now it cannot because WebKit1 and WebKit2 need to be able to
> coexist in the UI process,
> // and you can only have one WebCore::MemoryPressureHandler.
> 
> We can though add it directly there specifically with #if PLATFORM(GTK) and
> drop the added memory pressure handler.

We can make this cross-platform in webkit anyway, just move the check to the caller as I'm suggesting for the GTK case.

> The rest: sure
Comment 96 Alexander Mikhaylenko 2019-02-08 00:02:19 PST 
> Would other kind of gestures also need a setting? Should we make this more generic then? I mean future gestures.

Tbh I don't think there will be any more WebKit-side gestures. There are potential gestures for browsers in general, like swipe to switch tabs, or pull to refresh, but that doesn't involve WebKit.

There can be a variant of the same gesture for touchscreens though (say, an edge swipe), but it can use the same setting.

As for the existing ones, the touchscreen ones (pan, long press etc) are completely necessary, so I don't think it makes sense to be able to disable them. And pinch zoom (on both devices) is more debatable, but why would an app disable it?
Comment 97 Tim Horton 2019-02-08 00:12:35 PST 
(In reply to Alexander Mikhaylenko from comment #96)
> > Would other kind of gestures also need a setting? Should we make this more generic then? I mean future gestures.
> 
> Tbh I don't think there will be any more WebKit-side gestures. There are
> potential gestures for browsers in general, like swipe to switch tabs, or
> pull to refresh, but that doesn't involve WebKit.
> 
> There can be a variant of the same gesture for touchscreens though (say, an
> edge swipe), but it can use the same setting.
> 
> As for the existing ones, the touchscreen ones (pan, long press etc) are
> completely necessary, so I don't think it makes sense to be able to disable
> them. And pinch zoom (on both devices) is more debatable, but why would an
> app disable it?

FWIW a lot of WebKit embeds on macOS don't want the pinch gesture. We have a separate bit for each (swipe and pinch).
Comment 98 Alexander Mikhaylenko 2019-02-08 01:12:52 PST 
Hm. https://developer.apple.com/documentation/webkit/wkwebview/1414983-allowsmagnification?language=objc this one?

There already is a pinch gesture that is non-optional in WebKitGTK+ though. And the touchscreen one has been available for quite some time now, though there hasn't been a release with the touchpad one yet.
Comment 99 Alexander Mikhaylenko 2019-02-08 09:59:21 PST 
Created attachment 361512 [details]
Patch
Comment 100 Build Bot 2019-02-08 10:02:30 PST Comment hidden (obsolete)
Comment 101 Alexander Mikhaylenko 2019-02-08 10:05:08 PST 
Addressed comment 93. I'm not completely sure that's what you meant to do with WebMemoryPressureHandler, so maybe still missing something.

Also, added a gtk_widget_queue_draw() call to PageClientImpl::didRemoveNavigationGestureSnapshot(), so that we redraw the widget immediately after removing snapshot (otherwise, there's a rare bug where snapshot is still displayed until you scroll/cause redraws otherwise)
Comment 102 Michael Catanzaro 2019-02-08 10:49:21 PST 
EWS is red
Comment 103 Alexander Mikhaylenko 2019-02-08 11:44:44 PST 
Created attachment 361519 [details]
Patch
Comment 104 Alexander Mikhaylenko 2019-02-08 11:45:20 PST 
(In reply to Michael Catanzaro from comment #102)
> EWS is red

Oh, right, there was one more thing to do in project.pbxproj
Comment 105 Build Bot 2019-02-08 11:46:36 PST Comment hidden (obsolete)
Comment 106 Alexander Mikhaylenko 2019-02-08 12:16:56 PST 
Created attachment 361522 [details]
Patch
Comment 107 Build Bot 2019-02-08 12:20:18 PST Comment hidden (obsolete)
Comment 108 Alexander Mikhaylenko 2019-02-08 12:47:08 PST 
Created attachment 361524 [details]
Patch
Comment 109 Build Bot 2019-02-08 12:50:46 PST Comment hidden (obsolete)
Comment 110 Alexander Mikhaylenko 2019-02-08 13:00:13 PST 
Created attachment 361527 [details]
Patch
Comment 111 Build Bot 2019-02-08 13:03:27 PST Comment hidden (obsolete)
Comment 112 Alexander Mikhaylenko 2019-02-08 13:13:28 PST 
Created attachment 361528 [details]
Patch
Comment 113 Build Bot 2019-02-08 13:16:23 PST Comment hidden (obsolete)
Comment 114 Alexander Mikhaylenko 2019-02-08 13:43:28 PST 
Ok, it builds now. Also, the patch doesn't add or remove any @no-unify for Cocoa anymore, so that's solved as well.
Comment 115 Michael Catanzaro 2019-02-08 17:01:34 PST 
Comment on attachment 361528 [details]
Patch

Let's try it.
Comment 116 WebKit Commit Bot 2019-02-08 17:29:29 PST 
Comment on attachment 361528 [details]
Patch

Clearing flags on attachment: 361528

Committed r241224: <https://trac.webkit.org/changeset/241224>
Comment 117 WebKit Commit Bot 2019-02-08 17:29:33 PST 
All reviewed patches have been landed.  Closing bug.
Comment 118 Carlos Garcia Campos 2019-02-09 00:42:47 PST 
(In reply to Alexander Mikhaylenko from comment #101)
> Addressed comment 93. I'm not completely sure that's what you meant to do
> with WebMemoryPressureHandler, so maybe still missing something.

Exactly what I meant, thanks!

> Also, added a gtk_widget_queue_draw() call to
> PageClientImpl::didRemoveNavigationGestureSnapshot(), so that we redraw the
> widget immediately after removing snapshot (otherwise, there's a rare bug
> where snapshot is still displayed until you scroll/cause redraws otherwise)
Comment 119 Alexander Mikhaylenko 2019-02-09 02:37:41 PST 
And a followup: https://bugs.webkit.org/show_bug.cgi?id=194472 :(