[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


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #179075|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #22 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-12-21 07:00:06 PST ---
(From update of attachment 179075)
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

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