Bug 76921 - [WK2] FindController should not assume that ports do not want to highlight text matches
Summary: [WK2] FindController should not assume that ports do not want to highlight te...
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: 76070
  Show dependency treegraph
 
Reported: 2012-01-24 09:34 PST by Sergio Villar Senin
Modified: 2012-01-25 00:54 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.12 KB, patch)
2012-01-24 09:39 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (4.19 KB, patch)
2012-01-24 10:36 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-24 09:34:51 PST
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.
Comment 1 Sergio Villar Senin 2012-01-24 09:39:37 PST
Created attachment 123752 [details]
Patch
Comment 2 Darin Adler 2012-01-24 09:58:45 PST
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?
Comment 3 Darin Adler 2012-01-24 09:59:03 PST
Patch seems OK, but the PLATFORM(MAC) special case seems unneeded.
Comment 4 Sergio Villar Senin 2012-01-24 10:23:07 PST
(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.
Comment 5 Darin Adler 2012-01-24 10:25:24 PST
(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.
Comment 6 Sergio Villar Senin 2012-01-24 10:36:55 PST
Created attachment 123765 [details]
Patch
Comment 7 Darin Adler 2012-01-24 13:45:40 PST
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?
Comment 8 Sergio Villar Senin 2012-01-25 00:47:42 PST
(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.
Comment 9 Sergio Villar Senin 2012-01-25 00:54:33 PST
Committed r105855: <http://trac.webkit.org/changeset/105855>