[Webkit-unassigned] [Bug 193919] [GTK] Implement back/forward touchpad gesture

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=193919

--- Comment #7 from Alexander Mikhaylenko <exalm7659 at gmail.com> ---
(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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190130/47f53448/attachment.html>