[GTK] REGRESSION (r122428) WebKit2APITests/TestWebKitFindController fails "next" test

[Simon Pena]

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.

[Simon Pena]

I'm taking a look at this atm.

[Simon Pena]

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]

Assigning the bug to Sergio, who knows the code better.

[Simon Pena]

Bug #91224 tracks the skipped API tests.

[Sergio Villar Senin]

Created attachment 154589 [details]
Patch

[WebKit Review Bot]

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]

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 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]

(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 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 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]

Committed r125533: <http://trac.webkit.org/changeset/125533>