Bug 77747

Summary: [WK2] FindController::hideFindUI should unmark highlighted text matches
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: WebKit2Assignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cgarcia, darin, gustavo, svillar, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
New version of the patch with a proper unit test cgarcia: review+

Sergio Villar Senin
Reported 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.
Attachments
Patch (1.59 KB, patch)
2012-02-03 09:42 PST, Sergio Villar Senin 		no flags
DetailsFormatted DiffDiff
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+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1 2012-02-03 09:42:02 PST
Created attachment 125354 [details] Patch
Sergio Villar Senin
Comment 2 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.
Sergio Villar Senin
Comment 3 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.
Darin Adler
Comment 4 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.
Sergio Villar Senin
Comment 5 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).
Sergio Villar Senin
Comment 6 2012-02-29 10:43:35 PST
Created attachment 129473 [details] New version of the patch with a proper unit test
WebKit Review Bot
Comment 7 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
Carlos Garcia Campos
Comment 8 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.
Sergio Villar Senin
Comment 9 2012-06-22 02:54:42 PDT
Committed r121015: <http://trac.webkit.org/changeset/121015>
Note You need to log in before you can comment on or make changes to this bug.