Bug 212327

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.

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.

Other than for tests, no.

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

I mean beginSimulatedSwipeInDirectionForTesting() and completeSimulatedSwipeInDirectionForTesting() which create events right now. The patch in bug 212298 looks like it may be enough, yes.

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.

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

Created attachment 429520 [details] Patch Restored shadows too, this should be ready.

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

Created attachment 429523 [details] Patch Oh whoops, forgot to #if the new constants.

Created attachment 429526 [details] Patch Fixed code style

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.

Created attachment 429857 [details] Patch

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

Created attachment 430944 [details] Patch Sure.

Created attachment 430945 [details] Patch Forgot to regenerate changelog, done.

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!

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