[webkit-reviews] review granted: [Bug 212327] [GTK4] Add support for navigation gestures : [Attachment 429526] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at gnome.org> has granted Alexander Mikhaylenko
<alexm at gnome.org>'s request for review:
Bug 212327: [GTK4] Add support for navigation gestures
https://bugs.webkit.org/show_bug.cgi?id=212327

Attachment 429526: Patch

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




--- Comment #12 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 429526
  --> https://bugs.webkit.org/attachment.cgi?id=429526
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.