Bug 77747 - [WK2] FindController::hideFindUI should unmark highlighted text matches
Summary: [WK2] FindController::hideFindUI should unmark highlighted text matches
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-03 09:37 PST by Sergio Villar Senin
Modified: 2012-06-22 02:54 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.59 KB, patch)
2012-02-03 09:42 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
New version of the patch with a proper unit test (3.03 KB, patch)
2012-02-29 10:43 PST, Sergio Villar Senin
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2012-02-03 09:37:50 PST
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.
Comment 1 Sergio Villar Senin 2012-02-03 09:42:02 PST
Created attachment 125354 [details]
Patch
Comment 2 Sergio Villar Senin 2012-02-06 07:45:59 PST
(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.
Comment 3 Sergio Villar Senin 2012-02-08 09:12:25 PST
(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.
Comment 4 Darin Adler 2012-02-08 09:15:12 PST
(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.
Comment 5 Sergio Villar Senin 2012-02-10 09:57:20 PST
(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).
Comment 6 Sergio Villar Senin 2012-02-29 10:43:35 PST
Created attachment 129473 [details]
New version of the patch with a proper unit test
Comment 7 WebKit Review Bot 2012-02-29 11:42:17 PST
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 8 Carlos Garcia Campos 2012-06-22 02:31:29 PDT
Comment on attachment 129473 [details]
New version of the patch with a proper unit test

Patch looks sane to me.
Comment 9 Sergio Villar Senin 2012-06-22 02:54:42 PDT
Committed r121015: <http://trac.webkit.org/changeset/121015>