[Webkit-unassigned] [Bug 193919] [GTK] Implement back/forward touchpad gesture
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Feb 3 13:24:03 PST 2019
https://bugs.webkit.org/show_bug.cgi?id=193919
Michael Catanzaro <mcatanzaro at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #360858|1 |0
is obsolete| |
--- Comment #55 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 360858
--> https://bugs.webkit.org/attachment.cgi?id=360858
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
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190203/8b9dd95b/attachment-0001.html>
More information about the webkit-unassigned
mailing list