Bug 16222 - [GTK] Implement inline search and highlighting of matching strings.
Summary: [GTK] Implement inline search and highlighting of matching strings.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Major
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2007-12-01 06:35 PST by Christian Dywan
Modified: 2007-12-19 16:24 PST (History)
4 users (show)

See Also:


Attachments
Implement the feature (4.19 KB, patch)
2007-12-01 06:40 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Fixed return values; implementation following the win port now (6.57 KB, patch)
2007-12-01 08:29 PST, Christian Dywan
aroben: review-
Details | Formatted Diff | Diff
search and highlighting logic in WebCore (7.72 KB, patch)
2007-12-02 09:55 PST, Christian Dywan
aroben: review-
Details | Formatted Diff | Diff
Fixed style issues, renamed string to text (7.75 KB, patch)
2007-12-02 11:53 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Address issues pointed out by Mark (8.60 KB, patch)
2007-12-05 10:37 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Changed api to allow separate toggling of highlighting (9.33 KB, patch)
2007-12-05 11:17 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
api update (4.27 KB, patch)
2007-12-10 04:04 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
api update, this time with webcore changes (7.92 KB, patch)
2007-12-10 04:09 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
update to keep in synch (7.92 KB, patch)
2007-12-17 01:03 PST, Christian Dywan
alp: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Dywan 2007-12-01 06:35:32 PST
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.
Comment 1 Christian Dywan 2007-12-01 06:40:00 PST
Created attachment 17622 [details]
Implement the feature

webkit_web_view_search_string
webkit_web_view_highlight_matches
webkit_web_view_clear_highlight
Comment 2 Xan Lopez 2007-12-01 06:49:29 PST
+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.
Comment 3 Christian Dywan 2007-12-01 08:29:11 PST
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.
Comment 4 Adam Roben (:aroben) 2007-12-01 10:44:05 PST
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.
Comment 5 Christian Dywan 2007-12-02 09:55:36 PST
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.
Comment 6 Adam Roben (:aroben) 2007-12-02 10:03:25 PST
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.
Comment 7 Jan Alonzo 2007-12-02 10:08:30 PST
(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? 
Comment 8 Christian Dywan 2007-12-02 11:53:02 PST
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.
Comment 9 Adam Roben (:aroben) 2007-12-02 12:12:24 PST
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.
Comment 10 Mark Rowe (bdash) 2007-12-05 08:18:19 PST
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.
Comment 11 Adam Roben (:aroben) 2007-12-05 09:25:20 PST
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.
Comment 12 Christian Dywan 2007-12-05 10:37:25 PST
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.
Comment 13 Christian Dywan 2007-12-05 11:17:59 PST
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.
Comment 14 Christian Dywan 2007-12-10 04:04:51 PST
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
Comment 15 Christian Dywan 2007-12-10 04:09:26 PST
Created attachment 17820 [details]
api update, this time with webcore changes

Talk about *half* done, updated to include changes to WebCore again.
Comment 16 Maciej Stachowiak 2007-12-16 16:57:36 PST
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?
Comment 17 Christian Dywan 2007-12-17 01:03:47 PST
Created attachment 17952 [details]
update to keep in synch
Comment 18 Alp Toker 2007-12-19 16:19:58 PST
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!