Summary: | [GTK] REGRESSION (r122428) WebKit2APITests/TestWebKitFindController fails "next" test | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Pena <spenap> | ||||
Component: | WebKit2 | Assignee: | Sergio Villar Senin <svillar> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cgarcia, gustavo, mrobinson, svillar, webkit.review.bot, zan | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Simon Pena
2012-07-12 05:52:26 PDT
I'm taking a look at this atm. Several tests ("next", "previous" and "hide") were failing after the changeset. Next and Previous now report just one result in their search_next and search_previous methods. I'm not sure if this how it should be (so the tests need to be updated) or if it is an actual regression. Once that the "next" and "previous" tests are fixed, "hide" fails, identifying the "originalPixbuf" and "unhighlightedPixbuf" as different. Adding a delay with .wait seems to fix it (so this could be a consequence of the different behaviour regarding markAllTextMatches and unmarkAllTextMatches), but again, I'm not sure about this being the proper fix. Assigning the bug to Sergio, who knows the code better. Bug #91224 tracks the skipped API tests. Created attachment 154589 [details]
Patch
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 So yeah, Simon's analysis is basically correct. After r122428 the FindController returns just 1 match for search_next()/search_prev() if the test is found, so we should update the expected behaviour in the test. Apart from that the hide() unit test was flaky because it isn't enough to wait for the "draw" signal once, we have to wait also until all the glib events are processed. Comment on attachment 154589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154589&action=review > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:69 > g_main_loop_run(m_mainLoop); > + while (g_main_context_iteration(g_main_context_default(), FALSE)) { } I think this means that you should rename the method here, waitUntilWebViewDrawSignal is no longer accurate. What events are you waiting for in particular? (In reply to comment #8) > (From update of attachment 154589 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154589&action=review > > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:69 > > g_main_loop_run(m_mainLoop); > > + while (g_main_context_iteration(g_main_context_default(), FALSE)) { } > > I think this means that you should rename the method here, waitUntilWebViewDrawSignal is no longer accurate. What events are you waiting for in particular? Well thing is that waiting for "draw" does not ensure that the actual contents are displayed, when I asked the gtk folks they told me that was the safest approach. What about waitUnitWebViewContentsDrawn? Comment on attachment 154589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154589&action=review >>> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:69 >>> + while (g_main_context_iteration(g_main_context_default(), FALSE)) { } >> >> I think this means that you should rename the method here, waitUntilWebViewDrawSignal is no longer accurate. What events are you waiting for in particular? > > Well thing is that waiting for "draw" does not ensure that the actual contents are displayed, when I asked the gtk folks they told me that was the safest approach. What about waitUnitWebViewContentsDrawn? Maybe you could use gdk_events_pending() to check whether there are gdk events to be processed Comment on attachment 154589 [details]
Patch
This patch fixes two different issues actually, one is the previous/next issue (you should also unskip the tests cases, btw) which looks good to me, and the other is the hide test case that is flaky. Could you please file a different bug report for the hide issue? Feel free to commit the prev/next part, unskipping also the test cases.
Committed r125533: <http://trac.webkit.org/changeset/125533> |