Bug 76921 – [WK2] FindController should not assume that ports do not want to highlight text matches

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

[Sergio Villar Senin]

WK2's FindController assumes that clients do not want to highlight text matches. That's most likely because Safari uses the ShowOverlay option to dim the contents of the web page and then it renders some highlight on the overlay rectangles.

But other ports implementing the Find API (like GTK, see https://bugs.webkit.org/show_bug.cgi?id=76070) might want to keep WK1 behaviour, i.e., to let WebCore highlight all the matches found.

[Sergio Villar Senin]

Created attachment 123752 [details]
Patch

[Darin Adler]

Comment on attachment 123752 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:115
> +#if PLATFORM(MAC)
> +        bool shouldShowHighlight = false;
> +#else
> +        bool shouldShowHighlight = options & FindOptionsShowHighlight;
> +#endif

What’s the reasoning behind this #if?

[Darin Adler]

Patch seems OK, but the PLATFORM(MAC) special case seems unneeded.

[Sergio Villar Senin]

(In reply to comment #2)
> (From update of attachment 123752 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123752&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/FindController.cpp:115
> > +#if PLATFORM(MAC)
> > +        bool shouldShowHighlight = false;
> > +#else
> > +        bool shouldShowHighlight = options & FindOptionsShowHighlight;
> > +#endif
> 
> What’s the reasoning behind this #if?

Well as I said in the bug description, Safari does not need highlight at all, as it implements it using WK2's showOverlay and I guess some code in Safari that performs the nice animation and highlighting. I just used the #if to ensure that Safari keeps working even if, by mistake, the showHighlight option is used. It's obviously not required (I can get rid of it) if mac port just does not expose that find option and does not use it.

[Darin Adler]

(In reply to comment #4)
> Well as I said in the bug description, Safari does not need highlight at all, as it implements it using WK2's showOverlay and I guess some code in Safari that performs the nice animation and highlighting.

WebKit on Mac is not just for Safari.

> I just used the #if to ensure that Safari keeps working even if, by mistake, the showHighlight option is used.

OK. I don’t think that’s a realistic fear, so we can just not worry about it.

> It's obviously not required (I can get rid of it) if mac port just does not expose that find option and does not use it.

Lets leave it out.

[Sergio Villar Senin]

Created attachment 123765 [details]
Patch

[Darin Adler]

Comment on attachment 123765 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:111
> +        bool shouldShowHighlight = options & FindOptionsShowHighlight;

Why have this code here outside the if statement if this is used only when shouldShowOverlay is true?

[Sergio Villar Senin]

(In reply to comment #7)
> (From update of attachment 123765 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123765&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/FindController.cpp:111
> > +        bool shouldShowHighlight = options & FindOptionsShowHighlight;
> 
> Why have this code here outside the if statement if this is used only when shouldShowOverlay is true?

True, it's just that I messed it a bit because I was working in both this bug and https://bugs.webkit.org/show_bug.cgi?id=76522. If the latter is approved then the if will no longer exist, but anyway I agree that for this particular bug it makes no sense to have it outside the if scope.

[Sergio Villar Senin]

Committed r105855: <http://trac.webkit.org/changeset/105855>