RESOLVED FIXED 193919
[GTK] Implement back/forward touchpad gesture
https://bugs.webkit.org/show_bug.cgi?id=193919
Summary [GTK] Implement back/forward touchpad gesture
Alice Mikhaylenko
Reported 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.
Attachments
Patch (87.02 KB, patch)
2019-01-28 13:02 PST, Alice Mikhaylenko
no flags
Archive of layout-test-results from ews115 for mac-highsierra (2.02 MB, application/zip)
2019-01-28 14:49 PST, EWS Watchlist
no flags
Patch (134.11 KB, patch)
2019-01-31 07:05 PST, Alice Mikhaylenko
no flags
Patch (134.39 KB, patch)
2019-01-31 07:12 PST, Alice Mikhaylenko
no flags
Patch (135.89 KB, patch)
2019-01-31 07:54 PST, Alice Mikhaylenko
no flags
Patch (135.96 KB, patch)
2019-01-31 08:19 PST, Alice Mikhaylenko
no flags
Patch (157.46 KB, patch)
2019-01-31 11:11 PST, Alice Mikhaylenko
no flags
Patch (162.07 KB, patch)
2019-01-31 12:22 PST, Alice Mikhaylenko
no flags
Patch (162.03 KB, patch)
2019-01-31 12:38 PST, Alice Mikhaylenko
no flags
Patch (162.03 KB, patch)
2019-01-31 13:22 PST, Alice Mikhaylenko
no flags
Patch (162.04 KB, patch)
2019-01-31 13:33 PST, Alice Mikhaylenko
no flags
Patch (162.05 KB, patch)
2019-01-31 13:55 PST, Alice Mikhaylenko
no flags
Patch (163.60 KB, patch)
2019-01-31 14:15 PST, Alice Mikhaylenko
no flags
Patch (163.59 KB, patch)
2019-01-31 14:35 PST, Alice Mikhaylenko
no flags
Patch (163.59 KB, patch)
2019-01-31 14:51 PST, Alice Mikhaylenko
no flags
Patch (164.49 KB, patch)
2019-01-31 15:32 PST, Alice Mikhaylenko
no flags
Patch (166.59 KB, patch)
2019-01-31 16:01 PST, Alice Mikhaylenko
no flags
Patch (163.65 KB, patch)
2019-02-01 00:16 PST, Alice Mikhaylenko
no flags
Patch (165.51 KB, patch)
2019-02-01 00:54 PST, Alice Mikhaylenko
no flags
Patch (165.58 KB, patch)
2019-02-01 04:27 PST, Alice Mikhaylenko
no flags
Patch (98.71 KB, patch)
2019-02-03 13:12 PST, Alice Mikhaylenko
no flags
Patch (98.61 KB, patch)
2019-02-03 13:17 PST, Alice Mikhaylenko
no flags
Patch (165.42 KB, patch)
2019-02-03 13:46 PST, Alice Mikhaylenko
no flags
Patch (165.37 KB, patch)
2019-02-04 03:09 PST, Alice Mikhaylenko
no flags
Patch (171.18 KB, patch)
2019-02-04 08:00 PST, Alice Mikhaylenko
no flags
Patch (171.10 KB, patch)
2019-02-04 10:53 PST, Alice Mikhaylenko
no flags
Archive of layout-test-results from ews123 for ios-simulator-wk2 (12.78 MB, application/zip)
2019-02-04 12:42 PST, EWS Watchlist
no flags
Patch (193.60 KB, patch)
2019-02-05 05:53 PST, Alice Mikhaylenko
no flags
Patch (193.65 KB, patch)
2019-02-05 06:15 PST, Alice Mikhaylenko
no flags
Patch (193.91 KB, patch)
2019-02-05 06:46 PST, Alice Mikhaylenko
no flags
Patch (194.78 KB, patch)
2019-02-05 06:57 PST, Alice Mikhaylenko
no flags
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.70 MB, application/zip)
2019-02-05 08:15 PST, EWS Watchlist
no flags
Patch (194.84 KB, patch)
2019-02-06 04:32 PST, Alice Mikhaylenko
no flags
Patch (193.16 KB, patch)
2019-02-06 04:47 PST, Alice Mikhaylenko
no flags
Patch (205.37 KB, patch)
2019-02-08 09:59 PST, Alice Mikhaylenko
no flags
Patch (205.79 KB, patch)
2019-02-08 11:44 PST, Alice Mikhaylenko
no flags
Patch (208.36 KB, patch)
2019-02-08 12:16 PST, Alice Mikhaylenko
no flags
Patch (208.35 KB, patch)
2019-02-08 12:47 PST, Alice Mikhaylenko
no flags
Patch (207.72 KB, patch)
2019-02-08 13:00 PST, Alice Mikhaylenko
no flags
Patch (205.15 KB, patch)
2019-02-08 13:13 PST, Alice Mikhaylenko
no flags
Alice Mikhaylenko
Comment 1 2019-01-28 13:02:03 PST
EWS Watchlist
Comment 2 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
Alice Mikhaylenko
Comment 3 2019-01-28 13:23:08 PST
This is also currently broken with AcceleratedBackingStoreWayland, see bug 193903
EWS Watchlist
Comment 4 2019-01-28 14:49:30 PST Comment hidden (obsolete)
EWS Watchlist
Comment 5 2019-01-28 14:49:32 PST Comment hidden (obsolete)
Michael Catanzaro
Comment 6 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.
Alice Mikhaylenko
Comment 7 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?
Alice Mikhaylenko
Comment 8 2019-01-30 07:37:52 PST
Just for reference, the patch from bug 193903 fixes the gesture as well :)
Michael Catanzaro
Comment 9 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
Adrian Perez
Comment 10 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 :)
Alice Mikhaylenko
Comment 11 2019-01-31 07:05:25 PST
Alice Mikhaylenko
Comment 12 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.
Alice Mikhaylenko
Comment 13 2019-01-31 07:12:10 PST
EWS Watchlist
Comment 14 2019-01-31 07:14:09 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 15 2019-01-31 07:54:13 PST
EWS Watchlist
Comment 16 2019-01-31 07:55:40 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 17 2019-01-31 08:19:26 PST
EWS Watchlist
Comment 18 2019-01-31 08:21:24 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 19 2019-01-31 11:11:46 PST
EWS Watchlist
Comment 20 2019-01-31 11:15:37 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 21 2019-01-31 12:22:27 PST
EWS Watchlist
Comment 22 2019-01-31 12:26:09 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 23 2019-01-31 12:38:12 PST
EWS Watchlist
Comment 24 2019-01-31 12:41:47 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 25 2019-01-31 13:22:47 PST
EWS Watchlist
Comment 26 2019-01-31 13:25:42 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 27 2019-01-31 13:33:06 PST
EWS Watchlist
Comment 28 2019-01-31 13:35:03 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 29 2019-01-31 13:55:19 PST
EWS Watchlist
Comment 30 2019-01-31 13:57:57 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 31 2019-01-31 14:15:05 PST
EWS Watchlist
Comment 32 2019-01-31 14:19:20 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 33 2019-01-31 14:35:12 PST
EWS Watchlist
Comment 34 2019-01-31 14:38:12 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 35 2019-01-31 14:51:57 PST
EWS Watchlist
Comment 36 2019-01-31 14:54:51 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 37 2019-01-31 15:32:47 PST
EWS Watchlist
Comment 38 2019-01-31 15:36:15 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 39 2019-01-31 16:01:59 PST
EWS Watchlist
Comment 40 2019-01-31 16:06:06 PST Comment hidden (obsolete)
Michael Catanzaro
Comment 41 2019-01-31 17:47:26 PST
Comment on attachment 360800 [details] Patch Careful, you've duplicated your changelog in Tools/ChangeLog
Alice Mikhaylenko
Comment 42 2019-02-01 00:16:21 PST
EWS Watchlist
Comment 43 2019-02-01 00:18:17 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 44 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. :/
Alice Mikhaylenko
Comment 45 2019-02-01 00:54:02 PST
EWS Watchlist
Comment 46 2019-02-01 00:56:23 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 47 2019-02-01 04:27:06 PST
Alice Mikhaylenko
Comment 48 2019-02-01 04:28:21 PST
Ok, this one should be good (aside from the code style script needing an update)
EWS Watchlist
Comment 49 2019-02-01 04:30:13 PST Comment hidden (obsolete)
Michael Catanzaro
Comment 50 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.
Alice Mikhaylenko
Comment 51 2019-02-03 13:12:02 PST
Alice Mikhaylenko
Comment 52 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.
Alice Mikhaylenko
Comment 53 2019-02-03 13:17:13 PST
Alice Mikhaylenko
Comment 54 2019-02-03 13:18:20 PST
Sorry for the churn, forgot to update changelog for the Wayland change :x
Michael Catanzaro
Comment 55 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
Michael Catanzaro
Comment 56 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. ;)
Alice Mikhaylenko
Comment 57 2019-02-03 13:46:46 PST
EWS Watchlist
Comment 58 2019-02-03 13:50:02 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 59 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
Alice Mikhaylenko
Comment 60 2019-02-04 03:09:25 PST
EWS Watchlist
Comment 61 2019-02-04 03:12:27 PST Comment hidden (obsolete)
Michael Catanzaro
Comment 62 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
Alice Mikhaylenko
Comment 63 2019-02-04 08:00:19 PST
EWS Watchlist
Comment 64 2019-02-04 08:04:25 PST Comment hidden (obsolete)
Tim Horton
Comment 65 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
Alice Mikhaylenko
Comment 66 2019-02-04 10:53:42 PST
Alice Mikhaylenko
Comment 67 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)
EWS Watchlist
Comment 68 2019-02-04 10:57:25 PST Comment hidden (obsolete)
EWS Watchlist
Comment 69 2019-02-04 12:42:22 PST Comment hidden (obsolete)
EWS Watchlist
Comment 70 2019-02-04 12:42:25 PST Comment hidden (obsolete)
Michael Catanzaro
Comment 71 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.
Tim Horton
Comment 72 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
Alice Mikhaylenko
Comment 73 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.
Alice Mikhaylenko
Comment 74 2019-02-05 05:53:13 PST
EWS Watchlist
Comment 75 2019-02-05 05:56:36 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 76 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 :)
Alice Mikhaylenko
Comment 77 2019-02-05 06:15:55 PST
EWS Watchlist
Comment 78 2019-02-05 06:19:31 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 79 2019-02-05 06:46:43 PST
EWS Watchlist
Comment 80 2019-02-05 06:50:53 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 81 2019-02-05 06:57:13 PST
EWS Watchlist
Comment 82 2019-02-05 07:00:38 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 83 2019-02-05 07:35:02 PST
Ok, the last one seems to build on all platforms.
Alice Mikhaylenko
Comment 84 2019-02-05 07:57:06 PST
Nevermind, it seems to actually break some tests rather than them being flaky.
EWS Watchlist
Comment 85 2019-02-05 08:15:45 PST Comment hidden (obsolete)
EWS Watchlist
Comment 86 2019-02-05 08:15:48 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 87 2019-02-06 04:32:28 PST
Alice Mikhaylenko
Comment 88 2019-02-06 04:47:56 PST
EWS Watchlist
Comment 89 2019-02-06 04:50:19 PST Comment hidden (obsolete)
Michael Catanzaro
Comment 90 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.
Michael Catanzaro
Comment 91 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!
Alice Mikhaylenko
Comment 92 2019-02-06 10:30:21 PST
Carlos Garcia Campos
Comment 93 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.
Alice Mikhaylenko
Comment 94 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
Carlos Garcia Campos
Comment 95 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
Alice Mikhaylenko
Comment 96 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?
Tim Horton
Comment 97 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).
Alice Mikhaylenko
Comment 98 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.
Alice Mikhaylenko
Comment 99 2019-02-08 09:59:21 PST
EWS Watchlist
Comment 100 2019-02-08 10:02:30 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 101 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)
Michael Catanzaro
Comment 102 2019-02-08 10:49:21 PST
EWS is red
Alice Mikhaylenko
Comment 103 2019-02-08 11:44:44 PST
Alice Mikhaylenko
Comment 104 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
EWS Watchlist
Comment 105 2019-02-08 11:46:36 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 106 2019-02-08 12:16:56 PST
EWS Watchlist
Comment 107 2019-02-08 12:20:18 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 108 2019-02-08 12:47:08 PST
EWS Watchlist
Comment 109 2019-02-08 12:50:46 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 110 2019-02-08 13:00:13 PST
EWS Watchlist
Comment 111 2019-02-08 13:03:27 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 112 2019-02-08 13:13:28 PST
EWS Watchlist
Comment 113 2019-02-08 13:16:23 PST Comment hidden (obsolete)
Alice Mikhaylenko
Comment 114 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.
Michael Catanzaro
Comment 115 2019-02-08 17:01:34 PST
Comment on attachment 361528 [details] Patch Let's try it.
WebKit Commit Bot
Comment 116 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>
WebKit Commit Bot
Comment 117 2019-02-08 17:29:33 PST
All reviewed patches have been landed. Closing bug.
Carlos Garcia Campos
Comment 118 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)
Alice Mikhaylenko
Comment 119 2019-02-09 02:37:41 PST
Note You need to log in before you can comment on or make changes to this bug.