Bug 76522 – DidFindString should be emitted even if FindOptionsShowOverlay is not enabled

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

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

Created attachment 122896 [details]
Patch

[Sergio Villar Senin]

Created attachment 123895 [details]
Patch

[Sergio Villar Senin]

Updating the patch after http://trac.webkit.org/changeset/105855

[Darin Adler]

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.

[Sergio Villar Senin]

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

[Sergio Villar Senin]

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

[Sergio Villar Senin]

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?

[Sergio Villar Senin]

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

[Sergio Villar Senin]

Created attachment 125350 [details]
Patch

[Darin Adler]

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.

[Sergio Villar Senin]

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

[Darin Adler]

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

[Sergio Villar Senin]

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

[Sergio Villar Senin]

Committed r109228: <http://trac.webkit.org/changeset/109228>