Bug 77747 – [WK2] FindController::hideFindUI should unmark highlighted text matches

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Sergio Villar Senin]

Since http://trac.webkit.org/changeset/105855 ports can request WebCore to highlight text matches. Things is that there is no way to unmark all those text matches using WK2 API. That's why I suggest to call unmarkAllTextMatches() in FindController::hideFindUI().

It was not needed before r105855 because the code was unconditionally not highlighting text matches.

[Sergio Villar Senin]

Created attachment 125354 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #1)
> Created an attachment (id=125354) [details]
> Patch

Do not take into account the comment in ChangeLog about the lack of tests, it's a copy&pasted from other bug. I'm still trying to figure out how to test this (maybe some kind of screenshot...). Suggestions welcomed.

[Sergio Villar Senin]

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=125354) [details] [details]
> > Patch
> 
> Do not take into account the comment in ChangeLog about the lack of tests, it's a copy&pasted from other bug. I'm still trying to figure out how to test this (maybe some kind of screenshot...). Suggestions welcomed.

Actually I am not sure that we could do a proper test case for this since the highlight/unhighlight should be covered by a layout test, but we cannot trigger that API from a layout test. Not sure what do you think about landing this one without a test.

[Darin Adler]

(In reply to comment #3)
> Actually I am not sure that we could do a proper test case for this since the highlight/unhighlight should be covered by a layout test, but we cannot trigger that API from a layout test.

The normal approach in this situation is to add the ability to trigger the API from a layout test.

> Not sure what do you think about landing this one without a test.

Usually not a good idea.

[Sergio Villar Senin]

(In reply to comment #4)
> (In reply to comment #3)
> > Actually I am not sure that we could do a proper test case for this since the highlight/unhighlight should be covered by a layout test, but we cannot trigger that API from a layout test.
> 
> The normal approach in this situation is to add the ability to trigger the API from a layout test.
> 
> > Not sure what do you think about landing this one without a test.
> 
> Usually not a good idea.

So finally I managed to implement a unit test for this new behaviour. That unit test is implemented in the last version of the patch for http://webkit.org/b/76070 but I left it commented. So if the patch for this bug is OK for you then we could land it after 76070 adding the uncommenting of the pending tests (similar to what is planed for http://webkit.org/b/76522).

[Sergio Villar Senin]

Created attachment 129473 [details]
New version of the patch with a proper unit test

[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

[Carlos Garcia Campos]

Comment on attachment 129473 [details]
New version of the patch with a proper unit test

Patch looks sane to me.

[Sergio Villar Senin]

Committed r121015: <http://trac.webkit.org/changeset/121015>