[webkit-reviews] review denied: [Bug 98270] [GTK][WK2] Add API to retrieve a snapshot from a webview : [Attachment 179075] [GTK][WK2] Add API to retrieve a snapshot from a webview https://bugs.webkit.org/show_bug.cgi?id=98270

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> 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 179075: [GTK][WK2] Add API to retrieve a snapshot from a webview    
    https://bugs.webkit.org/show_bug.cgi?id=98270
https://bugs.webkit.org/attachment.cgi?id=179075&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=179075&action=review


Sorry for the delay reviewing it :-( Looks mostly good, there are just a few
minor issues and lack of more tests.

> Source/WebKit2/UIProcess/API/gtk/WebKitError.h:141
> +    WEBKIT_SNAPSHOT_ERROR_FAILED = 799

FAILED_TO_CREATE maybe?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2879
> + **/

Use single * here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2883
> +    webkit_web_view_get_snapshot(webView, rect, options, cancellable,
callback, userData);

Move the common code to a helper funtion and call it from here, but create a
different async result, every async methods should have its corresponding
_finish method. Add webkit_web_view_get_snapshot_of_visible_area_finish()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2889
> + * @rect: a #GdkRectangle with the region to take the snapshot of

I guess this is in document coordinates?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2896
> + * Asynchronously retrieves a snapshot of the @web_view at @rect. If
> + * @rect is empty, this function works exactly like

Maybe the empty special case could return a snapshot of the whole page, since
we already have a convenient variant to get the visible area, this would make
easier to get the whole page without having to query its size first.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2902
> +void webkit_web_view_get_snapshot(WebKitWebView* webView, GdkRectangle rect,
WebKitSnapshotOptions options, GCancellable* cancellable, GAsyncReadyCallback
callback, gpointer userData)

The rectangle should be a pointer.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2925
> + * Returns: (transfer full) : a #cairo_surface_t with the retrieved snapshot


Extra space between ) and :

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2926
> + **/

Since * here too.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:154
> + * @WEBKIT_SNAPSHOT_OPTIONS_INCLUDE_SELECTION_HIGHLIGHTING: Whether to
include in the
> + * snapshot the highlight of the selected content.

Are you sure this is the selection and not the search results? I've tried the
patch and I get the selection rendered in both cases.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1131
> +    cairo_surface_destroy(surface);

We need more tests, for example, we should tests the snapshot options, and
check that the snapshot is different when there's something selected and the
include selection option is used. There's code to compare pixbufs in search
unit tests, comparing cairo surfaces should be easy too.

> Source/WebKit2/UIProcess/WebPageProxy.h:764
> +    void getSnapshot(PassRefPtr<ImageCallback>, WebCore::IntRect,
SnapshotOptions);

You should probably pass a const reference for the IntRect