[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 #191951|review?                     |review-
               Flag|                            |




--- Comment #34 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-03-07 05:55:28 PST ---
(From update of attachment 191951)
View in context: https://bugs.webkit.org/attachment.cgi?id=191951&action=review

This looks pretty good! I have some comments though, see below.

> Source/WebKit2/Shared/ImageOptions.h:46
> +enum SnapshotRegion {
> +    SnapshotRegionVisible = 0,
> +    SnapshotRegionFullDocument
> +};

This is GTK+ specific in cross platform code. I think we can add this to a gtk+ specific private header.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2936
> +void WebKitWebViewDidReceiveSnapshotOfVisibleArea(WebKitWebView* webView, uint64_t callbackID, WebKit::WebImage* webImage)

First W should be lowercase. Also the OfVisibleArea sounds weird, since there are region options. webKitWebViewDidReceiveSnapshot() ? You don't need to use WebKit:: here in the cpp.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2952
> +    g_simple_async_result_complete(result.get());

You should remove the result from the map when the operation is done. You can also get the result from the map with take() instead of get() and then you don't need to remove it.

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

Nit: extra empty line here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2964
> +    switch (region) {
> +    case WEBKIT_SNAPSHOT_REGION_VISIBLE:
> +        return SnapshotRegionVisible;
> +    case WEBKIT_SNAPSHOT_REGION_FULL_DOCUMENT:
> +        return SnapshotRegionFullDocument;
> +    }
> +    ASSERT_NOT_REACHED();

You can use COMPILE_ASSERT_MATCHING_ENUM to make sure the enum values are the same at compile time, and use the values directly without a conversion function.

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

Remove this extra line.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3008
> +    message.set(String::fromUTF8("CallbackID"), WebUInt64::create(GPOINTER_TO_UINT(result)));

I think it would be easier and more consistent to use uint64_t directly. Define a function to get the callback, something like:

uint64_t generateSnapshotCallbackID()
{
        static uint64_t uniqueCallbackID = 1;
        return uniqueCallbackID++;
}

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3010
> +    webView->priv->snapshotResultsMap.set(GPOINTER_TO_UINT(result), result);

The result is leaked here, because the map takes a reference. So, you can use a GRefPtr adopting the ref and pass .get() to the map.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1129
> +        && !g_strcmp0(const_cast<const char*>(reinterpret_cast<char*>(cairo_image_surface_get_data(s1))),

I don't think g_strcmp0 is correct in this case, this is not NULL terminated strings, but data. You can probably use memcmp instead.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1136
> +    cairo_surface_t* surface1;
> +    cairo_surface_t* surface2;

Declare this when first used.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1141
> +    surface1 = test->waitForSnapshot(WEBKIT_SNAPSHOT_REGION_VISIBLE, WEBKIT_SNAPSHOT_OPTIONS_NONE);

Does this returns an error too? It seems that finish() only returns NULL in case of error. Is it considered an error to ask for the snapshot of a visible region when the visible region is empty?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1155
> +    g_assert(cairo_image_surface_get_width(surface1) <= 50);
> +    g_assert(cairo_image_surface_get_height(surface1) <= 50);
> +    int width = cairo_image_surface_get_width(surface1);
> +    int height = cairo_image_surface_get_height(surface1);

Why not moving the assert after getting widget/height? Use g_assert_cmpint to compare integers.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1163
> +    g_assert(cairo_image_surface_get_width(surface1) == width);
> +    g_assert(cairo_image_surface_get_height(surface1) == height);

Use g_asser_cmpint

> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:201
> +void WebViewTest::SelectAll()

This should be selectAll()

> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.h:51
> +    void SelectAll();

selectAll()

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:106
> +        if (frameView) {

frameView is only used inside the if, you can do:

if (WebCore::FrameView* frameView = page->mainFrameView()) {

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