Bug 193919 – [GTK] Implement back/forward touchpad gesture

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alice Mikhaylenko]

This is a very nice-to-have feature, and the existing Cocoa/Mac implementation can be ported relatively simply. Patch incoming.

[Alice Mikhaylenko]

Created attachment 360363 [details]
Patch

[EWS Watchlist]

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

[Alice Mikhaylenko]

This is also currently broken with AcceleratedBackingStoreWayland, see bug 193903

[EWS Watchlist]

Comment on attachment 360363 [details]
Patch

Attachment 360363 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10927572

New failing tests:
compositing/backing/animate-into-view.html

[EWS Watchlist]

Created attachment 360378 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Michael Catanzaro]

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.

[Alice Mikhaylenko]

(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?

[Alice Mikhaylenko]

Just for reference, the patch from  bug 193903 fixes the gesture as well :)

[Michael Catanzaro]

(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

[Adrian Perez]

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 :)

[Alice Mikhaylenko]

Created attachment 360724 [details]
Patch

[Alice Mikhaylenko]

It's not finished, I haven't fixed cairo_t leak yet, and ViewSnapshotStore can most probably be shared as well.

[Alice Mikhaylenko]

Created attachment 360725 [details]
Patch

[EWS Watchlist]

Attachment 360725 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360726 [details]
Patch

[EWS Watchlist]

Attachment 360726 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360728 [details]
Patch

[EWS Watchlist]

Attachment 360728 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360745 [details]
Patch

[EWS Watchlist]

Attachment 360745 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewSnapshotStore.cpp:159:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
ERROR: Source/WebKit/UIProcess/ViewSnapshotStore.cpp:165:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
ERROR: Source/WebKit/UIProcess/ViewSnapshotStore.cpp:165:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 8 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360753 [details]
Patch

[EWS Watchlist]

Attachment 360753 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:75:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360755 [details]
Patch

[EWS Watchlist]

Attachment 360755 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360763 [details]
Patch

[EWS Watchlist]

Attachment 360763 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360765 [details]
Patch

[EWS Watchlist]

Attachment 360765 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360770 [details]
Patch

[EWS Watchlist]

Attachment 360770 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360780 [details]
Patch

[EWS Watchlist]

Attachment 360780 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360785 [details]
Patch

[EWS Watchlist]

Attachment 360785 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360789 [details]
Patch

[EWS Watchlist]

Attachment 360789 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360795 [details]
Patch

[EWS Watchlist]

Attachment 360795 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360800 [details]
Patch

[EWS Watchlist]

Attachment 360800 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 360800 [details]
Patch

Careful, you've duplicated your changelog in Tools/ChangeLog

[Alice Mikhaylenko]

Created attachment 360846 [details]
Patch

[EWS Watchlist]

Attachment 360846 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

> Careful, you've duplicated your changelog in Tools/ChangeLog

Oops. I admit I'm not checking the changelog until it builds. :/

[Alice Mikhaylenko]

Created attachment 360848 [details]
Patch

[EWS Watchlist]

Attachment 360848 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 360858 [details]
Patch

[Alice Mikhaylenko]

Ok, this one should be good (aside from the code style script needing an update)

[EWS Watchlist]

Attachment 360858 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Alex, who'd be a good Apple reviewer for this? This is another big patch like we have in bug #174816.

[Alice Mikhaylenko]

Created attachment 361024 [details]
Patch

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

Created attachment 361026 [details]
Patch

[Alice Mikhaylenko]

Sorry for the churn, forgot to update changelog for the Wayland change :x

[Michael Catanzaro]

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

[Michael Catanzaro]

Comment on attachment 360858 [details]
Patch

I actually reviewed the older version of your patch. I'll assume you didn't make it worse. ;)

[Alice Mikhaylenko]

Created attachment 361027 [details]
Patch

[EWS Watchlist]

Attachment 361027 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 7 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

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

[Alice Mikhaylenko]

Created attachment 361050 [details]
Patch

[EWS Watchlist]

Attachment 361050 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

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

[Alice Mikhaylenko]

Created attachment 361056 [details]
Patch

[EWS Watchlist]

Attachment 361056 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

(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

[Alice Mikhaylenko]

Created attachment 361074 [details]
Patch

[Alice Mikhaylenko]

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)

[EWS Watchlist]

Attachment 361074 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 361074 [details]
Patch

Attachment 361074 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11027698

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html

[EWS Watchlist]

Created attachment 361084 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Michael Catanzaro]

Don't worry about the test failure. Pretty sure you didn't break WebRTC. Tests are flaky sometimes.

[Tim Horton]

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

[Alice Mikhaylenko]

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

[Alice Mikhaylenko]

Created attachment 361184 [details]
Patch

[EWS Watchlist]

Attachment 361184 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:350:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

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 :)

[Alice Mikhaylenko]

Created attachment 361185 [details]
Patch

[EWS Watchlist]

Attachment 361185 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:350:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 361187 [details]
Patch

[EWS Watchlist]

Attachment 361187 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:350:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 361188 [details]
Patch

[EWS Watchlist]

Attachment 361188 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:350:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Ok, the last one seems to build on all platforms.

[Alice Mikhaylenko]

Nevermind, it seems to actually break some tests rather than them being flaky.

[EWS Watchlist]

Comment on attachment 361188 [details]
Patch

Attachment 361188 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11039810

New failing tests:
swipe/basic-cached-back-swipe.html
swipe/pushState-programmatic-back-while-swiping-crash.html
swipe/pushState-cached-back-swipe.html

[EWS Watchlist]

Created attachment 361190 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Alice Mikhaylenko]

Created attachment 361287 [details]
Patch

[Alice Mikhaylenko]

Created attachment 361288 [details]
Patch

[EWS Watchlist]

Attachment 361288 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:350:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

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.

[Michael Catanzaro]

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!

[Alice Mikhaylenko]

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

[Carlos Garcia Campos]

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.

[Alice Mikhaylenko]

> 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

[Carlos Garcia Campos]

(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

[Alice Mikhaylenko]

> 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?

[Tim Horton]

(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).

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

Created attachment 361512 [details]
Patch

[EWS Watchlist]

Attachment 361512 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:133:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:351:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

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)

[Michael Catanzaro]

EWS is red

[Alice Mikhaylenko]

Created attachment 361519 [details]
Patch

[Alice Mikhaylenko]

(In reply to Michael Catanzaro from comment #102)
> EWS is red

Oh, right, there was one more thing to do in project.pbxproj

[EWS Watchlist]

Attachment 361519 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:133:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:351:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 361522 [details]
Patch

[EWS Watchlist]

Attachment 361522 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:133:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:351:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 361524 [details]
Patch

[EWS Watchlist]

Attachment 361524 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:133:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:351:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 361527 [details]
Patch

[EWS Watchlist]

Attachment 361527 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:133:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:351:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Created attachment 361528 [details]
Patch

[EWS Watchlist]

Attachment 361528 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53:  _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:133:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/ViewGestureController.h:351:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 6 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Mikhaylenko]

Ok, it builds now. Also, the patch doesn't add or remove any @no-unify for Cocoa anymore, so that's solved as well.

[Michael Catanzaro]

Comment on attachment 361528 [details]
Patch

Let's try it.

[WebKit Commit Bot]

Comment on attachment 361528 [details]
Patch

Clearing flags on attachment: 361528

Committed r241224: <https://trac.webkit.org/changeset/241224>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

(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)

[Alice Mikhaylenko]

And a followup: https://bugs.webkit.org/show_bug.cgi?id=194472 :(