Bug 91083

Summary: [GTK] REGRESSION (r122428) WebKit2APITests/TestWebKitFindController fails "next" test
Product: WebKit Reporter: Simon Pena <spenap>
Component: WebKit2Assignee: 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 Flags
Patch cgarcia: review+, cgarcia: commit-queue-

Description Simon Pena 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.
Comment 1 Simon Pena 2012-07-12 05:53:01 PDT
I'm taking a look at this atm.
Comment 2 Simon Pena 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.
Comment 3 Simon Pena 2012-07-12 11:01:02 PDT
Assigning the bug to Sergio, who knows the code better.
Comment 4 Simon Pena 2012-07-17 00:33:59 PDT
Bug #91224 tracks the skipped API tests.
Comment 5 Sergio Villar Senin 2012-07-26 02:34:17 PDT
Created attachment 154589 [details]
Patch
Comment 6 WebKit Review Bot 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
Comment 7 Sergio Villar Senin 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.
Comment 8 Martin Robinson 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?
Comment 9 Sergio Villar Senin 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?
Comment 10 Carlos Garcia Campos 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
Comment 11 Carlos Garcia Campos 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.
Comment 12 Sergio Villar Senin 2012-08-14 02:28:37 PDT
Committed r125533: <http://trac.webkit.org/changeset/125533>