[Webkit-unassigned] [Bug 98270] [GTK][WK2] Add API to retrieve a snapshot from a webview

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #166901|review?                     |review-
               Flag|                            |




--- Comment #4 from Martin Robinson <mrobinson at webkit.org>  2012-10-03 16:31:27 PST ---
(From update of attachment 166901)
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(simple));
> +
> +    return data->snapshot.get();

This can just be: static_cast<GetSnapshotAsyncData*>(g_simple_async_result_get_op_res_gpointer(simple))->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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.