[webkit-reviews] review denied: [Bug 89810] [GTK] Test /webkit2/WebKitFindController/hide always fails in Xvfb : [Attachment 198694] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Apr 18 02:13:09 PDT 2013
Carlos Garcia Campos <cgarcia at igalia.com> has denied Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 89810: [GTK] Test /webkit2/WebKitFindController/hide always fails in Xvfb
https://bugs.webkit.org/show_bug.cgi?id=89810
Attachment 198694: Patch
https://bugs.webkit.org/attachment.cgi?id=198694&action=review
------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=198694&action=review
Looks good in general, thanks!
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:68
> - g_signal_connect_after(m_webView, "draw", G_CALLBACK(webViewDraw),
this);
> + webkit_web_view_get_snapshot(m_webView,
WEBKIT_SNAPSHOT_REGION_VISIBLE, WEBKIT_SNAPSHOT_OPTIONS_NONE, 0,
> + reinterpret_cast<GAsyncReadyCallback>(onSnapshotReady), this);
I think we can make this generic so that it can be used by other tests, adding
WebViewTest::snapshot(), for example. Instead of the visible region I think you
should get the whole document.
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:283
> + g_assert_cmpint(cairo_image_surface_get_width(firstSurface), ==,
cairo_image_surface_get_width(secondSurface));
> + g_assert_cmpint(cairo_image_surface_get_height(firstSurface), ==,
cairo_image_surface_get_height(secondSurface));
> + g_assert_cmpint(cairo_image_surface_get_stride(firstSurface), ==,
cairo_image_surface_get_stride(secondSurface));
You should not assert here, you either call this assertSurfacesEqual and assert
when they are different, or you leave it as cairoSurfaceEqual and assert on the
result. Note that there's a function for this already in TestWebKitWebView.cpp.
Move it to Test.h to make it shareable and use it from both places.
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:285
> + cairo_image_surface_get_width(firstSurface) *
cairo_image_surface_get_stride(firstSurface));
Sorry, I told you wrongly, it's height * stride, not width * stride.
More information about the webkit-reviews
mailing list