Bug 89810 – [GTK] Test /webkit2/WebKitFindController/hide always fails in Xvfb

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

It's flaky when running without Xvfb

[Manuel Rego Casasnovas]

Created attachment 198694 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 198694 [details]
Patch

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.

[Manuel Rego Casasnovas]

Created attachment 198714 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 198714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=198714&action=review

Much better! Just some minor issues and a memory leak.

> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:437
> +    m_surface = 0;

This leaks the surface, this is not a smart pointer. You should check if the poiner is valid can call cairo_surface_destroy.

> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:440
> +    return cairo_surface_reference(m_surface);

If you are keeping a reference, it's better to return your reference and let the caller decide whether she needs to hold a reference or not.

> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:443
> +gboolean WebViewTest::cairoSurfacesEqual(cairo_surface_t* s1, cairo_surface_t* s2)

Use bool instead of gboolean. This has nothing to do with WebView, you could add it as a static method to Test, as I proposed before.

[Manuel Rego Casasnovas]

Created attachment 198836 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Manuel Rego Casasnovas]

Created attachment 198889 [details]
Patch

Rename WebViewTest::waitForSnapshot() method to WebViewTest::getSnapshotAndWaitUntilReady().

[WebKit Commit Bot]

Comment on attachment 198889 [details]
Patch

Clearing flags on attachment: 198889

Committed r148788: <http://trac.webkit.org/changeset/148788>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.