[webkit-reviews] review denied: [Bug 98270] [GTK][WK2] Add API to retrieve a snapshot from a webview : [Attachment 191951] Patch

[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 191951: Patch
https://bugs.webkit.org/attachment.cgi?id=191951&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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()) {