Bug 16222 – [GTK] Implement inline search and highlighting of matching strings.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Christian Dywan]

We want to be able to search for text in a website and highlight matching strings.

WebKitMac and WebKitWin have WebView::searchFor.

The term 'search' can be found at several places in Gtk/ Glib, such as gtk_text_iter_forward_search, gtk_tree_view_set_enable_search or g_sequence_search_iter.

I suggest webkit_web_view_search_string. I chose the term 'string' because a website could just as well be searched for hyperlinks or images.

Would we want to have an equivalent for frames? Mac and Win don't. I imagine we don't want one either, but comments are of course welcome.

[Christian Dywan]

Created attachment 17622 [details]
Implement the feature

webkit_web_view_search_string
webkit_web_view_highlight_matches
webkit_web_view_clear_highlight

[Xan Lopez]

+gboolean webkit_web_view_search_string(WebKitWebView* webView, const gchar* string, gboolean forward, gboolean case_sensitive, gboolean wrap)
+{
+    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL);
+    g_return_val_if_fail(string, NULL);

The function returns boolean, so it's FALSE :)

+guint webkit_web_view_highlight_matches(WebKitWebView* webView, const gchar* string, gboolean case_sensitive, guint limit)
+{
+    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL);
+    g_return_val_if_fail(string, NULL);

And zero here I guess.

Looks good to me other than that.

[Christian Dywan]

Created attachment 17623 [details]
Fixed return values; implementation following the win port now

The return values are fixed.

Further testing showed that a more sophisticated implementation is required and in fact most of the relevant code from the Win port was taken.

[Adam Roben (:aroben)]

Comment on attachment 17623 [details]
Fixed return values; implementation following the win port now

I think it would be a good idea to move the code that was copied from the Windows port onto WebCore::Page, rather than copying it all here, since presumably other ports will want it, too.

[Christian Dywan]

Created attachment 17650 [details]
search and highlighting logic in WebCore

As suggested by aroben I moved the actual logic to WebCore::Page and the gtk api has appropriate wrappers to use it.

[Adam Roben (:aroben)]

Comment on attachment 17650 [details]
search and highlighting logic in WebCore

 207 static Frame *incrementFrame(Frame *curr, bool forward, bool wrapFlag)

The *s are misplaced here -- they should go next to the type.

 214 bool Page::findString(const String& target, bool forward, bool caseFlag, bool wrapFlag)

I think caseFlag should be called caseSensitive and wrapFlag should be called wrap or shouldWrap.

 244 uint Page::markAllMatchesForText(const String& target, bool caseSensitive, bool highlight, uint limit)

You should use unsigned instead of uint.

 249     uint matches = 0;

Ditto.

 102         bool findString(const String&, bool, bool, bool);

Can this method be const?

It would be nice to change some of the do-while loops into for loops, but that's optional for this patch.

r- to fix these issues, then let's get this in! Thanks for putting this down in WebCore.

[Jan Alonzo]

(In reply to comment #0)

> I suggest webkit_web_view_search_string. I chose the term 'string' because a
> website could just as well be searched for hyperlinks or images.

Hi Christian. What do you think of webkit_web_view_search_text? 

[Christian Dywan]

Created attachment 17654 [details]
Fixed style issues, renamed string to text

I fixed the style issues Adam pointed out. I left the loops for now.

> 102         bool findString(const String&, bool, bool, bool);

I think that must not be const because it may modify the selection and the focus.

> Hi Christian. What do you think of webkit_web_view_search_text?

In fact I looked through a few gtk functions again and 'text' is probably the appropriate term. So I renamed it.

[Adam Roben (:aroben)]

Comment on attachment 17654 [details]
Fixed style issues, renamed string to text

+/**
+ * webkit_web_view_search_text:
+ * @web_view: a #WebKitWebView
+ * @text: a string to look for
+ * @forward: wether to find forward or not
+ * @case_sensitive: wether to respect the case of text
+ * @wrap: wether to continue looking at the beginning after reaching the end
+ *
+ * Looks for a specified string inside #web_view.
+ *
+ * Return value: %TRUE on success or %FALSE on failure
+ */
+gboolean webkit_web_view_search_text(WebKitWebView* webView, const gchar* string, gboolean forward, gboolean caseSensitive, gboolean shouldWrap)

Do the names of parameters in the documentation need to match their names in the function definition?

+uint Page::markAllMatchesForText(const String& target, bool caseSensitive, bool highlight, unsigned limit)

This needs to return an unsigned as well.

r=me, but whoever lands this needs to fix the return type of markAllMatchesForText.

[Mark Rowe (bdash)]

Comment on attachment 17654 [details]
Fixed style issues, renamed string to text

The use of so many boolean parameters on Page::findString makes me uncomfortable.  It will make the call sites hard to interpret without referring to the headers.

The argument names in the header need to be named as their meanings are not at all obvious based on the context.

I think the !!s should be removed as they were a Windows-ism.

The "found" variable in Page::findString looks to be redundant and could be removed with a tiny bit of tweaking, without losing any clarity.

The ternary operator in Page::markAllMatchesForText could do with a little bit of cleanup -- the spacing looks non-standard.

[Adam Roben (:aroben)]

Here's the relevant SPI from the Mac port (WebDocumentInternal.h):

@protocol WebMultipleTextMatches <NSObject>
- (void)setMarkedTextMatchesAreHighlighted:(BOOL)newValue;
- (BOOL)markedTextMatchesAreHighlighted;
- (WebNSUInteger)markAllMatchesForText:(NSString *)string caseSensitive:(BOOL)caseFlag limit:(WebNSUInteger)limit;
- (void)unmarkAllTextMatches;
- (NSArray *)rectsForTextMatches;
@end

Perhaps we should consider the form of the API a bit more before landing it, since APIs are hard to change once they exist.

[Christian Dywan]

Created attachment 17719 [details]
Address issues pointed out by Mark

This patch addresses the issues pointed out by Mark.

As Adam points out we might want to discuss the currently proposed api. For instance highlighting can be toggled independently.

[Christian Dywan]

Created attachment 17722 [details]
Changed api to allow separate toggling of highlighting

I changed the api to separate marking of strings and highlighting optionally. This has the side effect of a slightly clearer api.

[Christian Dywan]

Created attachment 17819 [details]
api update

Just noticed the previous patch was only half up to date.

My current proposal provides these functions:

webkit_web_view_search_text (web_view, string, case_sensitive, forward, wrap)
webkit_web_view_mark_text_matches (web_view, string, case_sensitive, limit)
webkit_web_view_set_highlight_text_matches (web_view, highlight)
webkit_web_view_unmark_text_matches

[Christian Dywan]

Created attachment 17820 [details]
api update, this time with webcore changes

Talk about *half* done, updated to include changes to WebCore again.

[Maciej Stachowiak]

Looks like this does a reasonable job of reimplementing what the Mac API does (with the handy simplification that it doesn't have to consider the whole WebDocumentView mess). Someone needs to give approval for the API. Alp?

[Christian Dywan]

Created attachment 17952 [details]
update to keep in synch

[Alp Toker]

Comment on attachment 17952 [details]
update to keep in synch

r=me

I think we might want to re-visit this at some point since I'm not comfortable with the number of parameters and naming of search_text() but given Maciej's thumbs-up and the lack of conversation I think we can land this.

Thanks!