Bug 212327 - [GTK4] Add support for navigation gestures
Summary: [GTK4] Add support for navigation gestures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: GTK4
  Show dependency treegraph
 
Reported: 2020-05-24 05:53 PDT by Carlos Garcia Campos
Modified: 2021-06-09 05:36 PDT (History)
10 users (show)

See Also:


Attachments
Support snapshots for the navigation gesture on GTK4 (18.96 KB, patch)
2021-05-16 11:49 PDT, Alice Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (23.12 KB, patch)
2021-05-24 03:37 PDT, Alice Mikhaylenko
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (23.14 KB, patch)
2021-05-24 04:36 PDT, Alice Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (23.10 KB, patch)
2021-05-24 05:06 PDT, Alice Mikhaylenko
mcatanzaro: review+
mcatanzaro: commit-queue-
Details | Formatted Diff | Diff
Patch (24.46 KB, patch)
2021-05-27 01:35 PDT, Alice Mikhaylenko
mcatanzaro: review+
Details | Formatted Diff | Diff
Patch (24.46 KB, patch)
2021-06-08 23:55 PDT, Alice Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (24.74 KB, patch)
2021-06-09 00:02 PDT, Alice Mikhaylenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2020-05-24 05:53:54 PDT
.
Comment 1 Alice Mikhaylenko 2020-05-24 05:56:33 PDT
Ok, so the code doesn't use anything special there, and as long as I have the scroll events, I can work with that. If raw events can't be accessed, using whatever is the replacement should be fine too.
Comment 2 Carlos Garcia Campos 2020-05-24 06:05:13 PDT
The main problem is that you can't create GdkEvents in GTK4. I'm not sure if you need to create events for the navigation gestures.
Comment 3 Alice Mikhaylenko 2020-05-24 06:05:48 PDT
Other than for tests, no.
Comment 4 Carlos Garcia Campos 2020-05-24 06:10:25 PDT
If for tests you mean the WTR EventSender implementation, we are getting rid of the GdkEvents in bug #212298. I hope that's enough.
Comment 5 Alice Mikhaylenko 2020-05-24 06:26:37 PDT
I mean beginSimulatedSwipeInDirectionForTesting() and completeSimulatedSwipeInDirectionForTesting() which create events right now.

The patch in bug 212298 looks like it may be enough, yes.
Comment 6 Alice Mikhaylenko 2021-05-16 11:43:33 PDT
The basics were ported in https://bugs.webkit.org/show_bug.cgi?id=212324

There are 2 missing pieces: thumbnails and shadows.

Attaching a patch for thumbnails, but it doesn't do shadows yet.
Comment 7 Alice Mikhaylenko 2021-05-16 11:49:56 PDT
Created attachment 428798 [details]
Support snapshots for the navigation gesture on GTK4
Comment 8 Alice Mikhaylenko 2021-05-24 03:37:40 PDT
Created attachment 429520 [details]
Patch

Restored shadows too, this should be ready.
Comment 9 EWS Watchlist 2021-05-24 03:38:42 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 10 Alice Mikhaylenko 2021-05-24 04:36:27 PDT
Created attachment 429523 [details]
Patch

Oh whoops, forgot to #if the new constants.
Comment 11 Alice Mikhaylenko 2021-05-24 05:06:20 PDT
Created attachment 429526 [details]
Patch

Fixed code style
Comment 12 Michael Catanzaro 2021-05-26 12:44:09 PDT
Comment on attachment 429526 [details]
Patch

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

Hm, do you think we should create a bug to change webkit_web_view_get_snapshot_finish() to return a GdkTexture rather than a cairo_surface_t? It might be nice to remove cairo from WebKit's API?

(We'd of course want to do that in a separate bug, not here. It will be a bit hard because I think gtk-doc cannot handle conditional compilation. We will blow its little mind.)

Anyway, on to the review. I have only some nits:

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:330
> +static GskRenderNode* createTextureNode(GdkTexture *texture, FloatSize &size)

Style: GdkTexture* texture, FloatSize& size

Also I don't like transferring ownership via a raw pointer in C++. That should only happen in legacy code or when we call external library functions. Instead, here you should return a GRefPtr<GskRenderNode>. Just return adoptGRef() here instead of calling adoptGRef(createTextureNode(...)) elsewhere.

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:332
> +    GtkSnapshot* snapshot = gtk_snapshot_new();

I know it's only alive for four lines, and because we use -fno-exceptions there's no way it can be leaked here. But we still want to hold ownership in smart pointers whenever possible, even if only for a couple lines. So please add a GRefPtr for GtkSnapshot and use it here, then you can release() it on the last line and call gtk_snapshot_free_to_node() on that.

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:338
> +}

So this would become:

return adoptGRef(gtk_snapshot_free_to_node(snapshot.release()));

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:495
> +
> +        return;

Nit: we don't normally leave a blank line before return.

> Source/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk4.cpp:48
> +    return !!m_texture;

Nit: !! is often required in C, but here it's not doing anything. bools are either true or false, and the conversion from RefPtr to bool is implicit.

> Source/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk4.cpp:72
> +    // FIXME: Unfortunately we don't have a way to get size of a texture
> +    // in bytes, so we'll have to make something up. Let's assume that
> +    // pixel == 4 bytes.
> +    return width * height * 4;

Looking at its usage in the cross-platform ViewSnapshot.cpp, I think you can safely rename ::imageSizeInBytes to ::estimateImageSizeInBytes. You'd still need this comment to explain what's going on, but you could get rid of the "FIXME:" prefix.
Comment 13 Alice Mikhaylenko 2021-05-27 01:35:30 PDT
Created attachment 429857 [details]
Patch
Comment 14 Alice Mikhaylenko 2021-05-27 01:37:48 PDT
(In reply to Michael Catanzaro from comment #12)
> Hm, do you think we should create a bug to change
> webkit_web_view_get_snapshot_finish() to return a GdkTexture rather than a
> cairo_surface_t? It might be nice to remove cairo from WebKit's API?

Agree

> Also I don't like transferring ownership via a raw pointer in C++. That
> should only happen in legacy code or when we call external library
> functions. Instead, here you should return a GRefPtr<GskRenderNode>. Just
> return adoptGRef() here instead of calling adoptGRef(createTextureNode(...))
> elsewhere.

Or I can just get rid of that function. It's just one render node, might as well create it manually without GtkSnapshot like we already do for color nodes.

> Nit: we don't normally leave a blank line before return.
> Nit: !! is often required in C
> Looking at its usage in the cross-platform ViewSnapshot.cpp

Fixed
Comment 15 Michael Catanzaro 2021-05-27 07:09:19 PDT
Comment on attachment 429857 [details]
Patch

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

r=me, but we need an owner to approve the change imageSizeInBytes() -> estimateImageSizeInBytes()

> Source/WebKit/UIProcess/ViewSnapshotStore.h:89
> -    size_t imageSizeInBytes() const { return m_surface ? m_surface->totalBytes() : 0; }
> +    size_t estimateImageSizeInBytes() const { return m_surface ? m_surface->totalBytes() : 0; }

Hi Chris, we need an owner for this... can you approve this change please?

The problem is our new ViewSnapshotStore implementation doesn't actually know for sure how large its snapshots are -- that's hidden behind an abstraction layer -- so we have to guess. It means the cache size limit won't be perfectly respected if our guess is wrong. Won't make a difference for Apple ports since your "estimate" is not changed and will always be exact.
Comment 16 Michael Catanzaro 2021-06-07 04:37:48 PDT
Comment on attachment 429857 [details]
Patch

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

>> Source/WebKit/UIProcess/ViewSnapshotStore.h:89
>> +    size_t estimateImageSizeInBytes() const { return m_surface ? m_surface->totalBytes() : 0; }
> 
> Hi Chris, we need an owner for this... can you approve this change please?
> 
> The problem is our new ViewSnapshotStore implementation doesn't actually know for sure how large its snapshots are -- that's hidden behind an abstraction layer -- so we have to guess. It means the cache size limit won't be perfectly respected if our guess is wrong. Won't make a difference for Apple ports since your "estimate" is not changed and will always be exact.

Hi owners, we just need this function name change approved please.
Comment 17 Simon Fraser (smfr) 2021-06-08 21:18:31 PDT
Comment on attachment 429857 [details]
Patch

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

>>> Source/WebKit/UIProcess/ViewSnapshotStore.h:89
>>> +    size_t estimateImageSizeInBytes() const { return m_surface ? m_surface->totalBytes() : 0; }
>> 
>> Hi Chris, we need an owner for this... can you approve this change please?
>> 
>> The problem is our new ViewSnapshotStore implementation doesn't actually know for sure how large its snapshots are -- that's hidden behind an abstraction layer -- so we have to guess. It means the cache size limit won't be perfectly respected if our guess is wrong. Won't make a difference for Apple ports since your "estimate" is not changed and will always be exact.
> 
> Hi owners, we just need this function name change approved please.

I approve estimatedImageSizeInBytes() (note the rename).
Comment 18 Alice Mikhaylenko 2021-06-08 23:55:12 PDT
Created attachment 430944 [details]
Patch

Sure.
Comment 19 Alice Mikhaylenko 2021-06-09 00:02:00 PDT
Created attachment 430945 [details]
Patch

Forgot to regenerate changelog, done.
Comment 20 Michael Catanzaro 2021-06-09 03:34:06 PDT
Comment on attachment 430945 [details]
Patch

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

> Source/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk3.cppSource/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk.cpp:63
> +size_t ViewSnapshot::estimatedImageSizeInBytes() const

estimated it is. Thanks Simon!
Comment 21 EWS 2021-06-09 05:36:14 PDT
Committed r278657 (238639@main): <https://commits.webkit.org/238639@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430945 [details].