RESOLVED FIXED 91083
[GTK] REGRESSION (r122428) WebKit2APITests/TestWebKitFindController fails "next" test
https://bugs.webkit.org/show_bug.cgi?id=91083
Summary [GTK] REGRESSION (r122428) WebKit2APITests/TestWebKitFindController fails "ne...
Simon Pena
Reported 2012-07-12 05:52:26 PDT
ERROR:../../Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:168:void testFindControllerNext(FindControllerTest*, gconstpointer): assertion failed: (test->m_matchCount == 2) TEST: /home/slave/webkitgtk/gtk-linux-64-release/build/Tools/gtk/../Scripts/../../WebKitBuild/Release/Programs/WebKit2APITests/TestWebKitFindController... (pid=20393) /webkit2/WebKitFindController/getters: OK /webkit2/WebKitFindController/instance: OK /webkit2/WebKitFindController/text-found: OK /webkit2/WebKitFindController/text-not-found: OK /webkit2/WebKitFindController/match-count: OK /webkit2/WebKitFindController/max-match-count: OK /webkit2/WebKitFindController/next: FAIL After r122428 - <http://trac.webkit.org/changeset/122428>, this test fails.
Attachments
Patch (3.13 KB, patch)
2012-07-26 02:34 PDT, Sergio Villar Senin
cgarcia: review+
cgarcia: commit-queue-
Simon Pena
Comment 1 2012-07-12 05:53:01 PDT
I'm taking a look at this atm.
Simon Pena
Comment 2 2012-07-12 10:59:45 PDT
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.
Simon Pena
Comment 3 2012-07-12 11:01:02 PDT
Assigning the bug to Sergio, who knows the code better.
Simon Pena
Comment 4 2012-07-17 00:33:59 PDT
Bug #91224 tracks the skipped API tests.
Sergio Villar Senin
Comment 5 2012-07-26 02:34:17 PDT
WebKit Review Bot
Comment 6 2012-07-26 02:36:05 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
Sergio Villar Senin
Comment 7 2012-07-26 02:37:07 PDT
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.
Martin Robinson
Comment 8 2012-07-26 02:37:45 PDT
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?
Sergio Villar Senin
Comment 9 2012-07-26 02:47:03 PDT
(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?
Carlos Garcia Campos
Comment 10 2012-08-02 10:57:39 PDT
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
Carlos Garcia Campos
Comment 11 2012-08-03 02:29:37 PDT
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.
Sergio Villar Senin
Comment 12 2012-08-14 02:28:37 PDT
Note You need to log in before you can comment on or make changes to this bug.