Bug 76522 - DidFindString should be emitted even if FindOptionsShowOverlay is not enabled
Summary: DidFindString should be emitted even if FindOptionsShowOverlay is not enabled
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-01-18 01:31 PST by Sergio Villar Senin
Modified: 2012-02-29 10:15 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.97 KB, patch)
2012-01-18 02:55 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (3.21 KB, patch)
2012-01-25 01:01 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (3.14 KB, patch)
2012-02-03 09:27 PST, Sergio Villar Senin
darin: 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-01-18 01:31:46 PST
After talking to andersca on IRC it seems it was done like that due to how OS X works. It seems that Safari works anyway if we send the message in the absence of that flag, and it seems more correct. This way, non Mac ports do not need to enable the show overlay options to get signaled about occurrences of the searched string in the web view.
Comment 1 Sergio Villar Senin 2012-01-18 02:55:19 PST
Created attachment 122896 [details]
Patch
Comment 2 Sergio Villar Senin 2012-01-25 01:01:52 PST
Created attachment 123895 [details]
Patch
Comment 3 Sergio Villar Senin 2012-01-25 01:02:51 PST
Updating the patch after http://trac.webkit.org/changeset/105855
Comment 4 Darin Adler 2012-01-26 10:46:31 PST
Comment on attachment 123895 [details]
Patch

The question for me is not “Is Safari broken?” but rather “Are there callers who want to do a find but not makeAllMatchesForText”. The patch talks about emitting DidFindString, but the key difference here is that the old code could be used to search for the string once, but the new code searches the full page for all instances of the string.
Comment 5 Sergio Villar Senin 2012-01-26 11:24:04 PST
(In reply to comment #4)
> (From update of attachment 123895 [details])
> The question for me is not “Is Safari broken?” but rather “Are there callers who want to do a find but not makeAllMatchesForText”. The patch talks about emitting DidFindString, but the key difference here is that the old code could be used to search for the string once, but the new code searches the full page for all instances of the string.

You made a good point, IMO we have 2 different issues here:

1) DidFindString should always be emitted (if the text is found obviously), whether or not you have the ShowOverlay option. I guess that's something that should not be under discussion.

2) It's true that the patch I uploaded changes the behaviour in the sense you mention. What we could do is the following. If the showOverlay and/or showHighlight options are specified we could assume that the caller wants markAllMatches to be called, otherwise the findString call will just look for the next occurrence of the search string in the test.

What do you think?
Comment 6 Sergio Villar Senin 2012-01-27 01:38:35 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 123895 [details] [details])
> > The question for me is not “Is Safari broken?” but rather “Are there callers who want to do a find but not makeAllMatchesForText”. The patch talks about emitting DidFindString, but the key difference here is that the old code could be used to search for the string once, but the new code searches the full page for all instances of the string.
> 
> You made a good point, IMO we have 2 different issues here:
> 
> 1) DidFindString should always be emitted (if the text is found obviously), whether or not you have the ShowOverlay option. I guess that's something that should not be under discussion.

Well perhaps it's indeed under discussion. I was not taking into account that the signal carries the total number of matches. So I have know a different proposal.

1) Remove the matchCount argument from the DidFindString message. It'll be issued whenever findString finds the string.
2) Perform the markAllTextMatches() only when (shouldShowOverlay || shouldShowHighlight) <- I am not very happy with this because something from the view is driving a different behaviour, but we can live with it in order not to add too many find options
3) Issue DidCountStringMatches() in FindController::findString method if markAllTextMatches() was called to inform about the total number of matches (since DidFindString does not longer carry that information)
Comment 7 Sergio Villar Senin 2012-02-01 10:54:51 PST
There is also another interesting thing to consider. Mac port does not care much about unmarking text matches because highlight is not shown and the find UI (the overlay) is automatically hidden after a mouse click. The obvious choice seems to be calling unmarkAllTextMatches in FindController::hideFindUI. What do you think?
Comment 8 Sergio Villar Senin 2012-02-01 11:36:18 PST
(In reply to comment #6)
> Well perhaps it's indeed under discussion. I was not taking into account that the signal carries the total number of matches. So I have know a different proposal.
> 
> 1) Remove the matchCount argument from the DidFindString message. It'll be issued whenever findString finds the string.
> 2) Perform the markAllTextMatches() only when (shouldShowOverlay || shouldShowHighlight) <- I am not very happy with this because something from the view is driving a different behaviour, but we can live with it in order not to add too many find options
> 3) Issue DidCountStringMatches() in FindController::findString method if markAllTextMatches() was called to inform about the total number of matches (since DidFindString does not longer carry that information)

Actually changing the signal signature is not required. The API contract would just specify that the number of matches will be at most 1 if neither showOverlay nor showHighlight are specified because in those cases find will just look for the next appearance.
Comment 9 Sergio Villar Senin 2012-02-03 09:27:24 PST
Created attachment 125350 [details]
Patch
Comment 10 Darin Adler 2012-02-03 10:44:46 PST
Comment on attachment 125350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125350&action=review

> Source/WebKit2/ChangeLog:16
> +        No new tests required, as there is no change in functionality,
> +        which is already covered by editing/text-iterator/findString.html.

This is a bogus comment! The patch adds a new, untested feature, and so there should be a test for that feature. Saying that existing test results aren’t affected is good, but has no bearing on whether a new test should be added. We do require tests for code changes. The test might need to be only on the GTK platform or have some other restriction.
Comment 11 Sergio Villar Senin 2012-02-06 01:23:09 PST
(In reply to comment #10)
> (From update of attachment 125350 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125350&action=review
> 
> > Source/WebKit2/ChangeLog:16
> > +        No new tests required, as there is no change in functionality,
> > +        which is already covered by editing/text-iterator/findString.html.
> 
> This is a bogus comment! The patch adds a new, untested feature, and so there should be a test for that feature. Saying that existing test results aren’t affected is good, but has no bearing on whether a new test should be added. We do require tests for code changes. The test might need to be only on the GTK platform or have some other restriction.

Fair enough. I have another patch http://webkit.org/b/76070 that adds a bunch of API tests for the webkitgtk's WK2 Find API. Some of them will fail without this patch. So we'd likely land those API tests first with some of them commented, and then this patch will make them work. Does it make sense?
Comment 12 Darin Adler 2012-02-06 10:22:46 PST
(In reply to comment #11)
> I have another patch http://webkit.org/b/76070 that adds a bunch of API tests for the webkitgtk's WK2 Find API. Some of them will fail without this patch. So we'd likely land those API tests first with some of them commented, and then this patch will make them work. Does it make sense?

Sure. The way we’d normally do that is have this patch include the “uncommenting out tests” aspect as well, but of course that involves getting that other patch landed before this patch is reviewed.
Comment 13 Sergio Villar Senin 2012-02-06 10:32:44 PST
(In reply to comment #12)
> (In reply to comment #11)
> > I have another patch http://webkit.org/b/76070 that adds a bunch of API tests for the webkitgtk's WK2 Find API. Some of them will fail without this patch. So we'd likely land those API tests first with some of them commented, and then this patch will make them work. Does it make sense?
> 
> Sure. The way we’d normally do that is have this patch include the “uncommenting out tests” aspect as well, but of course that involves getting that other patch landed before this patch is reviewed.

Great. So once the other patch with the new API lands, I will update and land this one mentioning that it fixes some of the tests previously committed to the repos.
Comment 14 Sergio Villar Senin 2012-02-29 10:15:44 PST
Committed r109228: <http://trac.webkit.org/changeset/109228>