[Webkit-unassigned] [Bug 212327] [GTK4] Add support for navigation gestures

[bugzilla-daemon at webkit.org]

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

Michael Catanzaro <mcatanzaro at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mcatanzaro at gnome.org
 Attachment #429526|review?, commit-queue?      |review+, commit-queue-
              Flags|                            |

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

-- 
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/20210526/e122d3a9/attachment.htm>