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.
Created attachment 17622 [details] Implement the feature webkit_web_view_search_string webkit_web_view_highlight_matches webkit_web_view_clear_highlight
+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.
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 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.
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 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.
(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?
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 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 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.
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.
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.
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.
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
Created attachment 17820 [details] api update, this time with webcore changes Talk about *half* done, updated to include changes to WebCore again.
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?
Created attachment 17952 [details] update to keep in synch
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!