WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(16.11 KB, patch)
2013-04-18 03:55 PDT
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(18.15 KB, patch)
2013-04-19 03:13 PDT
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(19.55 KB, patch)
2013-04-19 09:00 PDT
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Manuel Rego Casasnovas
Comment 1
2013-04-18 01:35:47 PDT
Created
attachment 198694
[details]
Patch
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
Created
attachment 198714
[details]
Patch
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
Created
attachment 198836
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug