Bug 212327 – [GTK4] Add support for navigation gestures

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

[Carlos Garcia Campos]

.

[Alice Mikhaylenko]

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.

[Carlos Garcia Campos]

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.

[Alice Mikhaylenko]

Other than for tests, no.

[Carlos Garcia Campos]

If for tests you mean the WTR EventSender implementation, we are getting rid of the GdkEvents in bug #212298. I hope that's enough.

[Alice Mikhaylenko]

I mean beginSimulatedSwipeInDirectionForTesting() and completeSimulatedSwipeInDirectionForTesting() which create events right now.

The patch in bug 212298 looks like it may be enough, yes.

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

Created attachment 428798 [details]
Support snapshots for the navigation gesture on GTK4

[Alice Mikhaylenko]

Created attachment 429520 [details]
Patch

Restored shadows too, this should be ready.

[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 https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alice Mikhaylenko]

Created attachment 429523 [details]
Patch

Oh whoops, forgot to #if the new constants.

[Alice Mikhaylenko]

Created attachment 429526 [details]
Patch

Fixed code style

[Michael Catanzaro]

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.

[Alice Mikhaylenko]

Created attachment 429857 [details]
Patch

[Alice Mikhaylenko]

(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

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Simon Fraser (smfr)]

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

[Alice Mikhaylenko]

Created attachment 430944 [details]
Patch

Sure.

[Alice Mikhaylenko]

Created attachment 430945 [details]
Patch

Forgot to regenerate changelog, done.

[Michael Catanzaro]

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!

[EWS]

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