RESOLVED FIXED 89810
[GTK] Test /webkit2/WebKitFindController/hide always fails in Xvfb
https://bugs.webkit.org/show_bug.cgi?id=89810
Summary [GTK] Test /webkit2/WebKitFindController/hide always fails in Xvfb
Carlos Garcia Campos
Reported 2012-06-23 06:39:42 PDT
It's flaky when running without Xvfb
Attachments
Patch (9.43 KB, patch)
2013-04-18 01:35 PDT, Manuel Rego Casasnovas
no flags
Patch (16.11 KB, patch)
2013-04-18 03:55 PDT, Manuel Rego Casasnovas
no flags
Patch (18.15 KB, patch)
2013-04-19 03:13 PDT, Manuel Rego Casasnovas
no flags
Patch (19.55 KB, patch)
2013-04-19 09:00 PDT, Manuel Rego Casasnovas
no flags
Manuel Rego Casasnovas
Comment 1 2013-04-18 01:35:47 PDT
Carlos Garcia Campos
Comment 2 2013-04-18 02:11:27 PDT
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
Comment 3 2013-04-18 03:55:36 PDT
Carlos Garcia Campos
Comment 4 2013-04-18 09:25:40 PDT
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
Comment 5 2013-04-19 03:13:54 PDT
WebKit Commit Bot
Comment 6 2013-04-19 03:14:33 PDT
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
Comment 7 2013-04-19 09:00:15 PDT
Created attachment 198889 [details] Patch Rename WebViewTest::waitForSnapshot() method to WebViewTest::getSnapshotAndWaitUntilReady().
WebKit Commit Bot
Comment 8 2013-04-20 01:22:08 PDT
Comment on attachment 198889 [details] Patch Clearing flags on attachment: 198889 Committed r148788: <http://trac.webkit.org/changeset/148788>
WebKit Commit Bot
Comment 9 2013-04-20 01:22:11 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.