[webkit-reviews] review denied: [Bug 98270] [GTK][WK2] Add API to retrieve a snapshot from a webview : [Attachment 166901] First iteration of the patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Claudio Saavedra
<csaavedra at igalia.com>'s request for review:
Bug 98270: [GTK][WK2] Add API to retrieve a snapshot from a webview
https://bugs.webkit.org/show_bug.cgi?id=98270

Attachment 166901: First iteration of the patch
https://bugs.webkit.org/attachment.cgi?id=166901&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166901&action=review


Looks pretty good to me, but there's a memory leak.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2630
> +static void didGetSnapshot(WKImageRef snapshot, WKErrorRef, void *context)

Nit: The asterisk is in the wrong place here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2638
> +	   data->snapshot = WKImageCreateCairoSurface(snapshot);

You probably want to use adoptRef here, since I'm assuming that
WKImageCreateCairoSurface returns a new reference.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2671
> + * Finishes an asynchronous opreation started with
webkit_web_view_get_snapshot().

Nit: opreation -> operation

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2673
> + * Returns: a #cairo_surface_t with the retrieved snapshot

It makes sense to include a transfer annotation here and mention it
specifically in the documentation. Right now you aren't returning a new
reference, so if the user doesn't ref the result immediately it will be freed.
I imagine this didn't cause a crash for you because of the leak above.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2678
> +    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), FALSE);
> +    g_return_val_if_fail(G_IS_ASYNC_RESULT(result), FALSE);

Probably better to return 0 instead of FALSE here, just for the sake of
correctness. They are the same thing in the end, but it looks a little funky.
:)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2684
> +	   return FALSE;

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2688
> +    GetSnapshotAsyncData* data =
static_cast<GetSnapshotAsyncData*>(g_simple_async_result_get_op_res_gpointer(si
mple));
> +
> +    return data->snapshot.get();

This can just be:
static_cast<GetSnapshotAsyncData*>(g_simple_async_result_get_op_res_gpointer(si
mple))->snapshot.get();

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1269
> +    RefPtr<WebImage> snapshotImage = scaledSnapshotWithOptions(contentRect,
1, SnapshotOptionsShareable | SnapshotOptionsExcludeSelectionHighlighting);

It seems it could be useful to just expose scaledSnapshotWithOptions to the IPC
layer. Wouldn't that make something like
webkit_web_view_get_scaled_snapshot_with_options possible?