Bug 76070 – [GTK] [WK2] Add Find API

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Sergio Villar Senin]

Originally proposed here: https://lists.webkit.org/pipermail/webkit-gtk/2011-December/000854.html

[Sergio Villar Senin]

Created attachment 122044 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 122044 [details]
Patch

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

Not a full review, just a couple quick comments. I think we should seriously consider a friendlier name than WebKitFindContext.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:283
> +    unsigned wkFindOptions = 0;
> +
> +    if (findContextFlags & WEBKIT_FIND_CONTEXT_CASE_INSENSITIVE)
> +        wkFindOptions |= kWKFindOptionsCaseInsensitive;
> +    if (findContextFlags & WEBKIT_FIND_CONTEXT_AT_WORD_STARTS)
> +        wkFindOptions |= kWKFindOptionsAtWordStarts;
> +    if (findContextFlags & WEBKIT_FIND_CONTEXT_TREAT_MEDIAL_CAPITAL_AS_WORD_START)
> +        wkFindOptions |= kWKFindOptionsTreatMedialCapitalAsWordStart;
> +    if (findContextFlags & WEBKIT_FIND_CONTEXT_BACKWARDS)
> +        wkFindOptions |= kWKFindOptionsBackwards;
> +    if (findContextFlags & WEBKIT_FIND_CONTEXT_WRAP_AROUND)
> +        wkFindOptions |= kWKFindOptionsWrapAround;
> +    if (findContextFlags & WEBKIT_FIND_CONTEXT_SHOW_OVERLAY)
> +        wkFindOptions |= kWKFindOptionsShowOverlay;
> +    if (findContextFlags & WEBKIT_FIND_CONTEXT_SHOW_FIND_INDICATOR)
> +        wkFindOptions |= kWKFindOptionsShowFindIndicator;
> +

If you make the values the same as the WebKit2 API, you can simply do a cast. Just add ASSERT_MATCHING_ENUM for each value somewhere in the file.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.h:42
> + * WebKitFindContextFlags:

I prefer WebKitFindFlags.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.h:51
> + * @WEBKIT_FIND_CONTEXT_FLAGS_NONE: no search flags
> + * @WEBKIT_FIND_CONTEXT_CASE_INSENSITIVE: case insensitive search
> + * @WEBKIT_AT_WORD_STARTS: search only at the begining of the words
> + * @WEBKIT_FIND_CONTEXT_TREAT_MEDIAL_CAPITAL_AS_WORD_START: treat capital letters in the middle of words as word start
> + * @WEBKIT_FIND_CONTEXT_BACKWARDS: search backwards
> + * @WEBKIT_FIND_CONTEXT_WRAP_AROUND: if not present search will stop at the end of the document
> + * @WEBKIT_FIND_CONTEXT_SHOW_OVERLAY: show an overlay for each match
> + * @WEBKIT_FIND_CONTEXT_SHOW_FIND_INDICATOR: show the find indicator
> + **/

I think the descriptions should be expanded a bit. For instance, I don't understand what the difference is between the overlay and the find indicator. These should also begin with capital letters. Shouldn't these all be prefixed with WEBKIT_FIND_CONTEXT_FLAGS and not just WEBKIT_FIND_CONTEXT_FLAGS_NONE?

[Sergio Villar Senin]

(In reply to comment #3)
> (From update of attachment 122044 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122044&action=review
> 
> If you make the values the same as the WebKit2 API, you can simply do a cast. Just add ASSERT_MATCHING_ENUM for each value somewhere in the file.

Didn't know about the ASSERT_MATCHING_ENUM thing.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.h:42
> > + * WebKitFindContextFlags:
> 
> I prefer WebKitFindFlags.

I guess the final name would depend on keeping WebKitFindContext or choosing another one

> I think the descriptions should be expanded a bit. For instance, I don't understand what the difference is between the overlay and the find indicator. These should also begin with capital letters. Shouldn't these all be prefixed with WEBKIT_FIND_CONTEXT_FLAGS and not just WEBKIT_FIND_CONTEXT_FLAGS_NONE?

Not sure, I checked some other GNOME libraries and there are examples of both approaches.

[Martin Robinson]

(In reply to comment #4)

> > > Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.h:42
> > > + * WebKitFindContextFlags:
> > I prefer WebKitFindFlags.
> 
> I guess the final name would depend on keeping WebKitFindContext or choosing another one

I'm not sure about that. My suggestion was to remove Context if we renamed it WebKitFindTask, for instance, this would still be called WebKitFindFlags.


> > I think the descriptions should be expanded a bit. For instance, I don't understand what the difference is between the overlay and the find indicator. These should also begin with capital letters. Shouldn't these all be prefixed with WEBKIT_FIND_CONTEXT_FLAGS and not just WEBKIT_FIND_CONTEXT_FLAGS_NONE?
> 
> Not sure, I checked some other GNOME libraries and there are examples of both approaches.

I'm fairly certain we do this with consistency in WebKit2. If not, we should, I think.

[Carlos Garcia Campos]

Comment on attachment 122044 [details]
Patch

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

You need to rebase so that the patch applies on current git master.

> Source/WebKit2/UIProcess/API/gtk/WebKitDefines.h:41
>  typedef struct _WebKitWebLoaderClient        WebKitWebLoaderClient;
>  typedef struct _WebKitWebLoaderClientClass   WebKitWebLoaderClientClass;
>  
> +typedef struct _WebKitFindContext            WebKitFindContext;
> +typedef struct _WebKitFindContextClass       WebKitFindContextClass;
> +

This was only needed by the old loader client, and it has been removed already, so please move this to its own header like other objects.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:29
> +#include <webkit2/WebKitEnumTypes.h>

In private headers or .cpp files: Use the quoted form for header files of WebKit2

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:56
> +    WKPageRef wkPageRef;

Ref is used for the name type, for the variable name just use wkPage

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:66
> +    WEBKIT_FIND_CONTEXT(clientInfo)->priv->matchCount = matchCount;
> +    g_signal_emit(WEBKIT_FIND_CONTEXT(clientInfo), signals[FOUND_STRING], 0);

If you need to cast more than once, I prefer to use a variable.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:77
> +    WEBKIT_FIND_CONTEXT(clientInfo)->priv->matchCount = matchCount;
> +    g_signal_emit(WEBKIT_FIND_CONTEXT(clientInfo), signals[COUNTED_MATCHES], 0);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:100
> +static void webkit_find_context_get_property(GObject* object, guint propId, GValue* value, GParamSpec* paramSpec)

This should use WebKit coding style, since it's not generated.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:122
> +static void webkit_find_context_set_property(GObject* object, guint propId, const GValue* value, GParamSpec* paramSpec)

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:141
> +static void webkit_find_context_finalize(GObject* object)

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:164
> +                                    g_param_spec_string("search-text",

I would avoid the search- prefix, the object is called FindContext so I think it's abvious that "text" is the text to search.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:177
> +                                    g_param_spec_flags("search-flags",

I would use options or find-options instead of flags to make it clearer what the flags are for.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:187
> +     * The number of matches found for the given text in the  #WebKitWebView.

the  #WebKitWebView. -> Extra space there.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:191
> +                                    g_param_spec_uint("match-count",

I think "count" is common in webkit, but in gnome we usually n-, n-matches in this case

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:204
> +                                    g_param_spec_uint("max-match-count",

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:212
> +     * @find: the #WebKitFindContext

find -> find_context

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:220
> +        g_signal_new("found-string",

We use text for the property and string for signal, I think this should be found-text or even just found.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:228
> +     * WebKitFindContext::failed-to-find-string:

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:229
> +     * @find: the #WebKitFindContext

find -> find_context

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:247
> +     * WebKitFindContext::counted-matches:

got-n-matches sounds more gnome to me too.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:248
> +     * @find: the #WebKitFindContext

find -> find_context

>> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:283
>> +
> 
> If you make the values the same as the WebKit2 API, you can simply do a cast. Just add ASSERT_MATCHING_ENUM for each value somewhere in the file.

Do we support all of those options?

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:289
> + * @findContext: the #WebKitFindContext

findCopntext -> find_context

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:298
> +    g_return_val_if_fail(WEBKIT_IS_FIND_CONTEXT(findContext), NULL);

Use 0 instead of NULL

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:304
> + * webkit_find_context_get_match_count:

webkit_find_context_get_n_matches() sounds more gnome.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:305
> + * @findContext: the #WebKitFindContext

findContext -> find_context

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:323
> + * webkit_find_context_get_flags:

I think get options, or get option_flags would be more clear

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:331
> +WebKitFindContextFlags
> +webkit_find_context_get_flags(WebKitFindContext* findContext)

This should be a single line

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:340
> + * @findContext: the #WebKitFindContext

findContext -> find_context

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:357
> + * @findContext: the #WebKitFindContext

findContext -> find_context

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:362
> + * "found-string" and "failed-to-find-string".

Use WebKitFindContext::found-string and WebKitFindContext::failed-to-find-string

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:368
> +    WKFindOptions findOptions = findContextFlagsToWKFindOptions(findContext->priv->searchFlags);

findOptions -> wkFindOptions

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:369
> +    WKRetainPtr<WKStringRef> searchTextRef(AdoptWK, WKStringCreateWithUTF8CString(findContext->priv->searchText.data()));

searchTextRef -> wkSearchText

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:375
> + * @findContext: the #WebKitFindContext

findContext -> find_context

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:379
> + * connected to "found-string" and/or "failed-to-find-string" signals

Use WebKitFindContext::signal-name here too

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:392
> + * webkit_find_context_prev:

prev-> previous

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:397
> + * connected to "found-string" and/or "failed-to-find-string" signals

Use WebKitFindContext::signal-name here too

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:410
> + * webkit_find_context_count_matches:

get_n_matches sounds more gnome to me

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:417
> + * "counted-matches" signal.

Use WebKitFindContext::signal-name here too

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:423
> +    WKFindOptions findOptions = findContextFlagsToWKFindOptions(findContext->priv->searchFlags);

findOptions -> wkFindOptions

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:424
> +    WKRetainPtr<WKStringRef> searchTextRef(AdoptWK, WKStringCreateWithUTF8CString(findContext->priv->searchText.data()));

searchTextRef -> wkSeachText

>>> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.h:42
>>> + * WebKitFindContextFlags:
>> 
>> I prefer WebKitFindFlags.
> 
> I guess the final name would depend on keeping WebKitFindContext or choosing another one

I prefer WebKitFindFlags too, even if the object is called Context, Taks, Operation, ...

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.h:61
> +  WEBKIT_FIND_CONTEXT_CASE_INSENSITIVE =                   (1 << 0),
> +  WEBKIT_FIND_CONTEXT_AT_WORD_STARTS =                     (1 << 1),
> +  WEBKIT_FIND_CONTEXT_TREAT_MEDIAL_CAPITAL_AS_WORD_START = (1 << 2),
> +  WEBKIT_FIND_CONTEXT_BACKWARDS =                          (1 << 3),
> +  WEBKIT_FIND_CONTEXT_WRAP_AROUND =                        (1 << 4),
> +  WEBKIT_FIND_CONTEXT_SHOW_OVERLAY =                       (1 << 5),
> +  WEBKIT_FIND_CONTEXT_SHOW_FIND_INDICATOR =                (1 << 6)

We don't need the parentheses.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.h:63
> +
> +} WebKitFindContextFlags;

Extra line there.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.h:92
> +WEBKIT_API const gchar*
> +webkit_find_context_get_search_string                        (WebKitFindContext        *find_context);
> +

Too many spaces between function name and parameters, just use one space for the longer function name and line up all others. And the same for type name and parameter.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1055
> +WebKitFindContext* webkit_web_view_search(WebKitWebView *webView,
> +                                          const gchar *searchText,
> +                                          WebKitFindContextFlags flags,
> +                                          guint maxMatchCount)

This should be a single line. API documentation is missing for this method. This still looks weird to me, what do you think about creating the find object for a given view instead of the view creating the find object?

WebKitFindContext *webkit_find_context_new(WebKitWebView *web_view);
This creates a find context to search on web view. You can connect to the signals in this moment and then.
void webkit_find_context_search(WebKitFindContext *find_context, const gchar *text, ......
...

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1058
> +    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL);
> +    g_return_val_if_fail(searchText, NULL);

NULL -> 0

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1063
> +    GRefPtr<WebKitFindContext> findContext = WEBKIT_FIND_CONTEXT(g_object_new(WEBKIT_TYPE_FIND_CONTEXT,

We don't need to use GRefPtr here.

[Martin Robinson]

(In reply to comment #6)
> > Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:191
> > +                                    g_param_spec_uint("match-count",
> 
> I think "count" is common in webkit, but in gnome we usually n-, n-matches in this case
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFindContext.cpp:204
> > +                                    g_param_spec_uint("max-match-count",
> 
> Ditto.
> 

"count" is pretty common in GLib and GTK+ as well (I just did a quick devhelp search). In this case I think it's friendlier and I'm not sure if there is a good way to use the n- version with max-match-count.

[Sergio Villar Senin]

Created attachment 122626 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 122626 [details]
Patch

Attachment 122626 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11254182

[Carlos Garcia Campos]

Comment on attachment 122626 [details]
Patch

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

In general I prefer to use "text" instead of "string", but I'm not opposed to use string anyway if you think it's better.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:26
> +#include "webkit2/WebKitEnumTypes.h"

#include "WebkitEnumTypes.h" should work.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:278
> + * @findController: the #WebKitFindController

This should be @find_controller, doc names should match the names used in the headers.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:281
> + * #WebKitWebView when a client calls webkit_web_view_search().

webkit_web_view_search doesn't exist now with the find controller.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:283
> + * Returns: (transfer none): the string to look for in the #WebKitWebView.

No need to use transfer none for methods returning const gchar. I would say the #WebKitWebView associated to @find_controller or something like that.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:284
> + **/

Double * is only needed at tthe beginning of the comment, not at the end.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:299
> + * webkit_web_view_search() call) this function returns G_MAXUINT.

webkit_web_view_search again.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:303
> + **/

Use single * here.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:318
> + **/

Single *

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:332
> + * calls webkit_web_view_search().

webkit_web_view_search

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:346
> + * @searchString: the string to look for

@search_string

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:347
> + * @findOptions: the #WebKitFindOptions used in the search

find_options

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:348
> + * @maxMatchCount: the maximum number of matches allowed in the search

max_match_count

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:351
> + * Look for @searchString in the #WebKitWebView associated with this
> + * #WebKitFindController. The outcome of the search will be

search_string. assciated with @find_controller maybe?

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:353
> + * asynchronously provided by the "WebKitFindController::found-string"
> + * and "WebKitFindController::failed-to-find-string".

No need to use "" here for signal references

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:365
> + **/

Single *

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:383
> + * webkit_web_view_search() in the #WebKitWebView. Callers must be

webkit_web_view_search

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:384
> + * connected to "found-string" and/or "failed-to-find-string" signals

Use WebKitFindController:signal-name

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:386
> + **/

Single *

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:401
> + * webkit_web_view_search() in the #WebKitWebView. Callers must be

webkit_web_view_search

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:402
> + * connected to "found-string" and/or "failed-to-find-string" signals

Use WebKitFindController::signal-name

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:404
> + **/

Single *

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:417
> + * @searchString: the string to look for

search_string

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:418
> + * @findOptions: the #WebKitFindOptions used in the search

find_options or just options

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:419
> + * @maxMatchCount: the maximum number of matches allowed in the search

max_match_count

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:425
> + * "WebKitFindController::counted-matches" signal.

Remove ""

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:426
> + **/

Single *

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.h:53
> + **/

Single *

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:239
> +    webView->priv->findController = 0;

glib already initializes the memory to 0 when allocating the instance struct, and GRefPtr initilizes the raw pointer to 0 in its constructor.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1214
> + * @webView: the #WebKitWebView

web_view

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1221
> + **/

Single *

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1228
> +        WebKitWebViewBase* webViewBase = WEBKIT_WEB_VIEW_BASE(webView);
> +        WebPageProxy* page = webkitWebViewBaseGetPage(webViewBase);

webViewBase is onlyn used here, so we can reduce this to a single line

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1230
> +        webView->priv->findController = WEBKIT_FIND_CONTROLLER(g_object_new(WEBKIT_TYPE_FIND_CONTROLLER, NULL));

Use adoptGRef() here.

> Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:77
> +webkit_web_view_search

webkit_web_view_get_find_controller

> Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:303
> +WebKitFindController

Add WebKitFindOptions here too

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:29
> +class FindControllerTest: public LoadTrackingTest {

what do you need LoadTrackingTest for? I whink this could be a WebView test

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:46
> +        // If the SHOW_OVERLAY option is not passed the WebPage does not notify about matches.

why? is that behaviour documented? we should make clear in the documentation what every find option is for.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:48
> +        m_findController = webkit_web_view_get_find_controller(m_webView);

The controller is always the same for the view, so initialize it in the constructor. Call assertObjectIsDeletedWhenTestFinishes() in the constructor to make sure it's not leaked.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:181
> +    webkit_find_controller_next(test->m_findController.get());

I guess this should be previous

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:186
> +    g_assert(!(webkit_find_controller_get_options(test->m_findController.get()) & WEBKIT_FIND_OPTIONS_BACKWARDS));

Doesn't this assert? shouldn't this be FORWARD instead of BACWARDS?

[Sergio Villar Senin]

(In reply to comment #10)
> (From update of attachment 122626 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122626&action=review
> 
> In general I prefer to use "text" instead of "string", but I'm not opposed to use string anyway if you think it's better.

No strong preference, just used string because it's also used in the C API.

[...]

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1230
> > +        webView->priv->findController = WEBKIT_FIND_CONTROLLER(g_object_new(WEBKIT_TYPE_FIND_CONTROLLER, NULL));
> 
> Use adoptGRef() here.

Yeah, stupid mistake

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:29
> > +class FindControllerTest: public LoadTrackingTest {
> 
> what do you need LoadTrackingTest for? I whink this could be a WebView test

Because I need to know when the find operation finishes. LoadTrackingTest checks that either find-string or failed-to-find-string signals are received.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:46
> > +        // If the SHOW_OVERLAY option is not passed the WebPage does not notify about matches.
> 
> why? is that behaviour documented? we should make clear in the documentation what every find option is for.

It is not documented AFAIK. I discovered it while implementing the unit tests. If you take a look at FindController::findString it's easy to see that DidFindString is only emitted when the show_overlay option is present. Anyway I still need to figure out what do each option exactly mean (a couple of them are not obvious to me).

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:48
> > +        m_findController = webkit_web_view_get_find_controller(m_webView);
> 
> The controller is always the same for the view, so initialize it in the constructor. Call assertObjectIsDeletedWhenTestFinishes() in the constructor to make sure it's not leaked.

Good point.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:181
> > +    webkit_find_controller_next(test->m_findController.get());
> 
> I guess this should be previous

The idea was to do a next() + previous(). How can we test previous() without a next(), that's yin and yang :P

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:186
> > +    g_assert(!(webkit_find_controller_get_options(test->m_findController.get()) & WEBKIT_FIND_OPTIONS_BACKWARDS));
> 
> Doesn't this assert? shouldn't this be FORWARD instead of BACWARDS?

No, note that there is a "!" before webkit_find_controller_get_options. There is no FORWARD flag, either you have the BACKWARDS flag or you haven't it (which means forward).

[Carlos Garcia Campos]

Comment on attachment 122626 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:29
>>> +class FindControllerTest: public LoadTrackingTest {
>> 
>> what do you need LoadTrackingTest for? I whink this could be a WebView test
> 
> Because I need to know when the find operation finishes. LoadTrackingTest checks that either find-string or failed-to-find-string signals are received.

That's checked by your FindControllerTest, not LoadTrackingText. I think it could inherit from WebViewTest instead of LoadTRackingTest.

[Carlos Garcia Campos]

Comment on attachment 122626 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.h:79
> +    /* signals */
> +    void (* found_string)             (WebKitFindController    *find_controller);
> +    void (* failed_to_find_string)    (WebKitFindController    *find_controller);
> +    void (* counted_matches)          (WebKitFindController    *find_controller);

You are not supposed to be able to inherit from FindController, because there's no way to set a custom find controller to the view, and find controller can only be created by a web view, so I think we don't need virtual methods for the signals.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.h:85
> +  /* Padding for future expansion */
> +    void (*_webkit_reserved0)         (void);
> +    void (*_webkit_reserved1)         (void);
> +    void (*_webkit_reserved2)         (void);
> +    void (*_webkit_reserved3)         (void);

And we can remove the padding here, since we won't add virtual methods.

[Sergio Villar Senin]

After talking to andersca we agreed that showOverlay must not be a requirement to get DidFindString signaling. Adding a dependency, will update this patch once the other lands.

[Sergio Villar Senin]

Created attachment 122900 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #15)
> Created an attachment (id=122900) [details]
> Patch

Rebased against 76522 and addressing latest Carlos' comments.

[Gustavo Noronha (kov)]

Comment on attachment 122900 [details]
Patch

Attachment 122900 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11281152

[Carlos Garcia Campos]

Comment on attachment 122900 [details]
Patch

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

Now that you have removed the overlay and indicator options, does this still depend on bug #76522? Code looks good to me in general, see some minor comments below. But I still have some concerns about the API:

 - I still think we should use text instead of string: in gnome API string is commonly used as type name, like in g_value_set_string, g_settings_set_string, etc. and text is used as text shown in the UI like in gtk_entry_set_text, gtk_label_get_text, gtk_cell_renderer_text_new, etc. We can probably avoid string/text in some cases like webkit_find_controller_search() since it's obvious you are searching text, the same way we have next/previous and not next_string/previous_string.

 - A find controller is associated to a web view, but once it's returned by the web view, it's not possible to know the web view the find controller is associated to. API docs talks about the WebKitWebView all the time, but from the WebKitFindController POV there's not web view. What do you think about keeping the web view instead of the WKPage (you can then get the page from the web view)? It would be a construct only property. This way we don't need webkitFindAttachFindClientToPage, we can do it in the constructed method of WebKitFindController. And of course, we would add webkit_find_controller_get_web_view().

 - string, match-count and options are read-write properties, but the API only expose getters for them, and they are only set by webkit_find_controller_search_string() and webkit_find_controller_count_matches(). I would make those properties read-only and add set those manually instead of using g_object_set. This way we also avoid setting the string and max-match-count in next/previous methods.

 - match-count is a property and can be updated by a search operation in didFindString() or webkit_find_controller_count_matches() in didCountStringMatches(), but we are not emitting GObject::notify signal when it's updated. If we connect to counted-matches we will only be notified when updated from didCountStringMatches(). What do you think about removing counted-matrches signal and emit notify everytime the property is updated?

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:332
> + **/

Single * here.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:347
> + * Look for @searchString in the #WebKitWebView associated with

@search_string

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:353
> + * with the same find options use webkit_find_controller_search_next()

Method is called webkit_find_controller_next() not webkit_find_controller_search_next() which makes me think whether it should be webkit_find_controller_search_next() indeed.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:354
> + * and/or webkit_find_options_search_previous(). The

webkit_find_options_search_previous() -> webkit_find_controller_previous() or webkit_find_controller_search_previous() if we renamed it.

> Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:330
> +<SECTION>
> +<FILE>WebKitFindOptions</FILE>
> +webkit_find_options_get_type
> +</SECTION>

We don't use a new section for enum types that are specific to a section, just add it to the FindController section.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:27
> +#define TEST_STRING "<html><body>first testing second testing secondHalf</body></html>"

Use static const char*

[Sergio Villar Senin]

(In reply to comment #18)
> (From update of attachment 122900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122900&action=review
> 
> Now that you have removed the overlay and indicator options, does this still depend on bug #76522?

Yes, because without it, we wouldn't receive the didFindString signal unless the overlay find option is passed to the search method.

>  - I still think we should use text instead of string: in gnome API string is commonly used as type name, like in g_value_set_string, g_settings_set_string, etc. and text is used as text shown in the UI like in gtk_entry_set_text, gtk_label_get_text, gtk_cell_renderer_text_new, etc. We can probably avoid string/text in some cases like webkit_find_controller_search() since it's obvious you are searching text, the same way we have next/previous and not next_string/previous_string.

Not strong opinion here, but we better reach an agreement before I go on changing names forever.

>  - A find controller is associated to a web view, but once it's returned by the web view, it's not possible to know the web view the find controller is associated to. API docs talks about the WebKitWebView all the time, but from the WebKitFindController POV there's not web view. What do you think about keeping the web view instead of the WKPage (you can then get the page from the web view)? It would be a construct only property. This way we don't need webkitFindAttachFindClientToPage, we can do it in the constructed method of WebKitFindController. And of course, we would add webkit_find_controller_get_web_view().

Looks like a very good idea. Do you think we should have a getter for it?

>  - string, match-count and options are read-write properties, but the API only expose getters for them, and they are only set by webkit_find_controller_search_string() and webkit_find_controller_count_matches(). I would make those properties read-only and add set those manually instead of using g_object_set. This way we also avoid setting the string and max-match-count in next/previous methods.

Definitely.
 
>  - match-count is a property and can be updated by a search operation in didFindString() or webkit_find_controller_count_matches() in didCountStringMatches(), but we are not emitting GObject::notify signal when it's updated. If we connect to counted-matches we will only be notified when updated from didCountStringMatches(). What do you think about removing counted-matrches signal and emit notify everytime the property is updated?

Looks appealing to me.

[Martin Robinson]

Comment on attachment 122900 [details]
Patch

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

This looks great to me. The only big change I would make is to remove the match-count property and simply return the value with the signal. It means that you only have to do one thing to get the match count instead of multiple things.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:54
> +    WebKitFindOptions findOptions;

findOptions needs to be a guint, since it's a bitmask.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:87
> +static void didCountStringMatches(WKPageRef page, WKStringRef string, unsigned matchCount, const void* clientInfo)
> +{
> +    WebKitFindController* findController = WEBKIT_FIND_CONTROLLER(clientInfo);
> +    findController->priv->matchCount = matchCount;
> +    g_signal_emit(findController, signals[COUNTED_MATCHES], 0);
> +}

I think it makes sense to get rid of the match-count property and simply return the value with this signal. It's not really a stateful thing, but rather the return value of an asycnhronous method call.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:169
> +     * The text to search in the #WebKitWebView.

Should be "The text to search for"

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:175
> +                                                        _("Text to search in the view"),

Should be Text to search for

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:224
> +     * page text. It will be issued (if the text is found)

No need for parenthesis here.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:241
> +     * any result for the given string. It will be issued (if the text
> +     * is not found) asynchronously after a call to

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:260
> +     * This signal is emitted to inform about the number of matches
> +     * found in the #WebKitWebView for the given string.  It will be
> +     * issued asynchronously after a call to
> +     * webkit_find_controller_count_matches().

I would suggest slightly different wording here. Perhaps:

This signal is emitted when the #WebKitFindController has counted the number of matches of matches for a given string after a call to webkit_find_controller_count_matches().

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:275
> + * Get the string to look for. This string is passed to either

I think the first sentence could be:

Gets the string that @find_controller is currently searching for. This is the string passed to either...

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:295
> + * #WebKitWebView. If the number of matches is higher than the maximum
> + * number of matches allowed (the last parameter to the
> + * webkit_web_view_search() call) this function returns G_MAXUINT.

Again, I think this can be something like:

Asynchronously query the number of matches for the last search text.

Does the behavior of returning G_MAXUINT comes from the C API or WebCore?

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:311
> + * Get the options used to tune up the find operation.

You should probably say something like:

Gets a bitmask containing the #WebKitFindOptions associated with the current search.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:313
> + * Returns: the find options.

Returns: a bitmask containing the #WebKitFindOptions associated with the current search.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:315
> +WebKitFindOptions webkit_find_controller_get_options(WebKitFindController* findController)

This actually needs to return a guint, since it's a bitmask.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:345
> + * @max_match_count: the maximum number of matches allowed in the search

Might want to mention how to have no limit?

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:363
> +void webkit_find_controller_search_string(WebKitFindController* findController, const gchar* searchString, WebKitFindOptions findOptions, guint maxMatchCount)

findOptions needs to be a guint, since it' a bitmask.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:385
> + * webkit_find_controller_search_string() or
> + * webkit_find_controller_count_matches() in the
> + * #WebKitWebView. Callers must be connected to
> + * WebKitFindController::found-string and/or
> + * WebKitFindController::failed-to-find-string signals to get the
> + * result of the search.

You can probably  just say: Look for the next occurence of the search string.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:402
> + * webkit_find_controller_search_string() or
> + * webkit_find_controller_count_matches() in the

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:291
> +    FindControllerTest::add("WebKitFindController", "getters", testFindControllerGetters);
> +    FindControllerTest::add("WebKitFindController", "instance", testFindControllerInstance);
> +    FindControllerTest::add("WebKitFindController", "text-found", testFindControllerTextFound);
> +    FindControllerTest::add("WebKitFindController", "text-not-found", testFindControllerTextNotFound);
> +    FindControllerTest::add("WebKitFindController", "match-count", testFindControllerMatchCount);
> +    FindControllerTest::add("WebKitFindController", "max-match-count", testFindControllerMaxMatchCount);
> +    FindControllerTest::add("WebKitFindController", "next", testFindControllerNext);
> +    FindControllerTest::add("WebKitFindController", "previous", testFindControllerPrevious);
> +    FindControllerTest::add("WebKitFindController", "counted-matches", testFindControllerCountedMatches);
> +    FindControllerTest::add("WebKitFindController", "options", testFindControllerOptions);

I really like how your seperated all assertions into different tests. This makes debugging failures a lot easier!

[Carlos Garcia Campos]

I think it's perfectly valid to use WebKitFindOptions in public API instead do guint.

[Sergio Villar Senin]

Created attachment 123184 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 123184 [details]
Patch

Attachment 123184 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11307121

[Sergio Villar Senin]

I still have doubts about this API. Matches are not highlighted, only the current match is selected but not highlighted. There is some kind of highlight if we use the show overlay FindOption that I removed from my last patch. Thing is that I am not sure that we would want/need the overlay thing (the current rendering is pretty bad anyway). Also the API does not allow callers to unmark the highlighted matches (if we'd have the highlight).

[Carlos Garcia Campos]

(In reply to comment #24)
> I still have doubts about this API. Matches are not highlighted, only the current match is selected but not highlighted. There is some kind of highlight if we use the show overlay FindOption that I removed from my last patch. Thing is that I am not sure that we would want/need the overlay thing (the current rendering is pretty bad anyway). Also the API does not allow callers to unmark the highlighted matches (if we'd have the highlight).

We could try to use overlay option internally without exposing it in the API to get the behaviour we want. I think that the find should just work like we are used to, all matches are highlighted when you start a search operation, with the current match using a different color or opacity. When the search operation finishes, matches are not highlighted anymore. So, instead or mark/unmark we could just add _cancel() or _finish().

[Martin Robinson]

(In reply to comment #25)
> (In reply to comment #24)
> > I still have doubts about this API. Matches are not highlighted, only the current match is selected but not highlighted. There is some kind of highlight if we use the show overlay FindOption that I removed from my last patch. Thing is that I am not sure that we would want/need the overlay thing (the current rendering is pretty bad anyway). Also the API does not allow callers to unmark the highlighted matches (if we'd have the highlight).
> 
> We could try to use overlay option internally without exposing it in the API to get the behaviour we want. I think that the find should just work like we are used to, all matches are highlighted when you start a search operation, with the current match using a different color or opacity. When the search operation finishes, matches are not highlighted anymore. So, instead or mark/unmark we could just add _cancel() or _finish().

I think we need to figure out what was happening in WebCore in WebKit1 and make sure we can do the same thing in WebKit2. The WebKit layer shouldn't change the behavior here, since all the logic is in WebCore.

[Sergio Villar Senin]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > I still have doubts about this API. Matches are not highlighted, only the current match is selected but not highlighted. There is some kind of highlight if we use the show overlay FindOption that I removed from my last patch. Thing is that I am not sure that we would want/need the overlay thing (the current rendering is pretty bad anyway). Also the API does not allow callers to unmark the highlighted matches (if we'd have the highlight).
> > 
> > We could try to use overlay option internally without exposing it in the API to get the behaviour we want. I think that the find should just work like we are used to, all matches are highlighted when you start a search operation, with the current match using a different color or opacity. When the search operation finishes, matches are not highlighted anymore. So, instead or mark/unmark we could just add _cancel() or _finish().
> 
> I think we need to figure out what was happening in WebCore in WebKit1 and make sure we can do the same thing in WebKit2. The WebKit layer shouldn't change the behavior here, since all the logic is in WebCore.

I explained the reasons in http://webkit.org/b/76522. Basically WK2 C API was unconditionally setting the showHighlight to false because the mac port does not use it (they use an overlay + some code in the different browsers). The relevant discussion continues in the bug mentioned above.

[Sergio Villar Senin]

(In reply to comment #27)
> > I think we need to figure out what was happening in WebCore in WebKit1 and make sure we can do the same thing in WebKit2. The WebKit layer shouldn't change the behavior here, since all the logic is in WebCore.
> 
> I explained the reasons in http://webkit.org/b/76522. Basically WK2 C API was unconditionally setting the showHighlight to false because the mac port does not use it (they use an overlay + some code in the different browsers). The relevant discussion continues in the bug mentioned above.

Ok, so I'm removing the r? flag at least until 76522 is fixed. The highlight thing was already fixed with 76921. I'm removing the r? flag because this API lacks the ability to unmark (unhighlight) text matches, something that was possible with WK1. After talking to Carlos we concluded that having something like webkit_find_controller_search_finish() in the API would be nice.

The implementation is quite straightforward but needs some changes in the WK2 internals that I'm currently discussing in 76522.

[Sergio Villar Senin]

Comment on attachment 123184 [details]
Patch

Removing the review flag. Will provide a more complete patch soon.

[Sergio Villar Senin]

Added bug 77747 to dependencies. It's the one that handles the unhighlight issue.

[Sergio Villar Senin]

Created attachment 125356 [details]
Patch

[Sergio Villar Senin]

Created attachment 125622 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #32)
> Created an attachment (id=125622) [details]
> Patch

As suggested by Martin I uploaded a new version of this patch that does not assume that 76522 was fixed before. This way unit tests need to artificially use a findOption that we do not even expose to work. Once this lands we could land also 76522 (that already has a r+) and then remove that hack from the unit tests.

[Martin Robinson]

Comment on attachment 125622 [details]
Patch

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

Looking pretty good! I'm going to try to fix the style bot to catch more of these errors.

> Source/WebKit2/UIProcess/API/gtk/WebKitDefines.h:51
> +typedef struct _WebKitFindController        WebKitFindController;
> +typedef struct _WebKitFindControllerClass   WebKitFindControllerClass;
> +
> +typedef struct _WebKitWebView WebKitWebView;
> +typedef struct _WebKitWebViewClass WebKitWebViewClass;

It's a bit odd that one of these is aligned and one isn't. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:55
> +    OP_FIND,
> +    OP_COUNT

Since these WebKit enums are private to the file, they should use WebKit naming conventions. Thus they should be FindOperation and CountOperation.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:61
> +    guint findOptions;
> +    unsigned maxMatchCount;

Why use guint and then unsigned? You should probably use "unsigned int" for both. I'd keep guint for the API, but avoid it internally.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:97
> +static WKPageRef inline getWKPageFromWebKitWebView(WebKitWebView *webView)

You asterisk is in the wrong place here.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:171
> +     * The text to search for in the #WebKitWebView.

Maybe something like "The current search string for this #WebKitFindController." would be more accurate?

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:217
> +                                                        _("Web View"),
> +                                                        _("The Web View associated with this find controller"),

I think we should use WebView here. This seems really inconsistent, but we always say WebView like we always say WebKit.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:224
> +     * @match_count: the number of matches of the search text

"the number of matches found in the search text" I think.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:353
> +        // We always highlight text matches

This comment is missing the "Why?" Why do you we always highlight text matches? Watch out for the missing period here.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:377
> + * Looks for @search_text in the #WebKitWebView associated with
> + * @find_controller. The outcome of the search will be asynchronously
> + * provided by the WebKitFindController::found-string and
> + * WebKitFindController::failed-to-find-string.

Might want to mention that this starts from the beginning of the document.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.h:79
> +WEBKIT_API void
> +webkit_find_controller_search              (WebKitFindController *find_controller,
> +					    const gchar          *search_text,
> +					    guint                 find_options,
> +					    guint                 max_match_count);
> +

The indentation looks off at some places in this file.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:35
> +        : m_runFindUntilCompletion(false),
> +          m_findController(webkit_web_view_get_find_controller(m_webView))

The comma goes at the start of the next line.

[Sergio Villar Senin]

(In reply to comment #34)
> (From update of attachment 125622 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125622&action=review
> 
> Looking pretty good! I'm going to try to fix the style bot to catch more of these errors.

[...]

Fixed all those style issues. 

> > Source/WebKit2/UIProcess/API/gtk/WebKitFindController.h:79
> > +WEBKIT_API void
> > +webkit_find_controller_search              (WebKitFindController *find_controller,
> > +					    const gchar          *search_text,
> > +					    guint                 find_options,
> > +					    guint                 max_match_count);
> > +
> 
> The indentation looks off at some places in this file.

Yeah some tabs were there by mistake.

Any other comment apart from those style fixes Martin?

[Carlos Garcia Campos]

Comment on attachment 125622 [details]
Patch

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

Looks great!

> Source/WebKit2/UIProcess/API/gtk/WebKitDefines.h:51
> +typedef struct _WebKitFindController        WebKitFindController;
> +typedef struct _WebKitFindControllerClass   WebKitFindControllerClass;
> +
> +typedef struct _WebKitWebView WebKitWebView;
> +typedef struct _WebKitWebViewClass WebKitWebViewClass;

I don't think we need forward declaration for class structs

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:56
> +typedef enum {
> +    OP_FIND,
> +    OP_COUNT
> +} WebKitFindControllerFindOperation;

Maybe WebKitFindControllerOperation? the second Find sounds a bit redundant to me and makes the name longer.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:77
> +    g_signal_emit(WEBKIT_FIND_CONTROLLER(clientInfo), signals[FOUND_STRING], 0, matchCount);

Can we use text instead of string everywhere? For the reasons I explained in comment #18. You said you don't have strong preference, so do you mind using text instead?

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:287
> +const char* webkit_find_controller_get_search_text(WebKitFindController* findController)

You are indeed using text here instead of string :-)

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.h:28
> +#include <webkit2/WebKitWebView.h>

That header doesn't have the WebView typedef anymore, so include #include <webkit2/WebKitDefines.h>.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.h:49
> + * @WEBKIT_FIND_OPTIONS_WRAP_AROUND: if not present search will stop at the end of the document
> + */

A brief explanation is missing here, a single line saying this an enum containing the flags used when searching.

[Sergio Villar Senin]

Created attachment 125877 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 125877 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:29
> +#include <WebKit2/WKFindOptions.h>
> +#include <WebKit2/WKRetainPtr.h>
> +#include <WebKit2/WKString.h>

Sorry, I forgot to say in previous review, but this should all be included by WebKitPrivate.h

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:169
> +     * WebKitFindController::text

Trailing : missing here

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:469
> + **/

Single * here

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:470
> +void webkit_find_controller_search_finish(WebKitFindController *findController)

* is incorrectly placed here, it should be WebKitFindController* findController

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1282
> +WebKitFindController* webkit_web_view_get_find_controller(WebKitWebView *webView)

WebKitWebView *webView -> WebKitWebView* webView

[Carlos Garcia Campos]

Comment on attachment 125877 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:169
>> +     * WebKitFindController::text
> 
> Trailing : missing here

And it's a property, it should be 

WebKitFindController:text:

[Sergio Villar Senin]

OK, I have fixed the remaining style issues locally. Anything else to consider? Are we all happy with the API?

[Carlos Garcia Campos]

(In reply to comment #40)
> OK, I have fixed the remaining style issues locally. Anything else to consider? Are we all happy with the API?

I am

[Sergio Villar Senin]

(In reply to comment #40)
> OK, I have fixed the remaining style issues locally. Anything else to consider? Are we all happy with the API?

So the thing is that, while reviewing the final patch for submitting I noticed that with this implementation, search_next() and search_previous() will be internally always unmarking + marking all text matches when they should just find the next/previous occurrence of the string. That's because http://webkit.org/b/78132.

That will make search for the next/previous slower than it should theoretically be (as it's doing more operations than it should), *but* at the same time, that issue shouldn't force us to change the API, perhaps we will have to change the implementation in another future bug, but the API should remain untouched.

[Sergio Villar Senin]

Created attachment 126528 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #43)
> Created an attachment (id=126528) [details]
> Patch

So this patch addresses the last style issues reported by Carlos&Martin and on top on that I added an additional test case for the last addition to the API webkit_find_controller_search_finish().

As it can be seen in the code the unit tests have a couple of comments that clearly (I hope so) specify that some stuff can not be tested until a couple of bugs are fixed:
* http://webkit.org/b/76522: Darin already gave a r+ with the condition of providing a tests case (that would mean uncomment some code in the unit tests)
* http://webkit.org/b/77747: I'll try to get a r+ the same way I got it for the other bug.

Hope everything is clear.

[Philippe Normand]

Comment on attachment 126528 [details]
Patch

Attachment 126528 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11480730

[Darin Adler]

Error on GTK EWS is:

TestWebKitFindController.cpp:274:102: error: 'gdk_pixbuf_get_pixels_with_length' was not declared in this scope

[Sergio Villar Senin]

Created attachment 126984 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #46)
> Error on GTK EWS is:
> 
> TestWebKitFindController.cpp:274:102: error: 'gdk_pixbuf_get_pixels_with_length' was not declared in this scope

Yes sorry, I was using locally a bleeding edge version of gdk-pixbuf

[WebKit Review Bot]

Attachment 126984 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 126984 [details]
Patch

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

Very nice. This patch looks fine, except that 3 seconds is *way* too long for a test to run. Please try to get the test to run within 1/2 second before landing. If possible replace the timeouts with a more deterministic approach.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:128
> +        g_value_set_string(value, findController->priv->searchText.data());
> +        break;
> +    case PROP_OPTIONS:
> +        g_value_set_uint(value, findController->priv->findOptions);
> +        break;
> +    case PROP_MAX_MATCH_COUNT:
> +        g_value_set_uint(value, findController->priv->maxMatchCount);
> +        break;
> +    case PROP_WEB_VIEW:
> +        g_value_set_object(value, findController->priv->webView);

Not a huge deal, but these should probably call the getter methods. This is consistent with other GObjects and allows for more complex behavior such as lazy caching of WebCore values.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:357
> +    } else
> +        WKPageCountStringMatches(wkPage, wkSearchText.get(), wkFindOptions, findController->priv->maxMatchCount);

You should use an early return here.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:391
> + * This function will also highligh up to @max_match_count

highligh -> highlight

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:310
> +    // WebCore has highlighted the matches or no, and there is also no

Nit: or no -> or not

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:336
> +    test->wait(1.0);
> +    GdkPixbuf* originalPixbuf = gdk_pixbuf_get_from_window(webViewGdkWindow, 0, 0, allocatedHeight, allocatedWidth);
> +    g_assert(originalPixbuf);
> +
> +    test->find("testing", WEBKIT_FIND_OPTIONS_NONE, 1);
> +    test->waitUntilFindFinished();
> +    g_assert(test->m_textFound);
> +
> +    test->wait(1.0);
> +    GdkPixbuf* highlightPixbuf = gdk_pixbuf_get_from_window(webViewGdkWindow, 0, 0, allocatedHeight, allocatedWidth);
> +    g_assert(highlightPixbuf);
> +    g_assert(!gdkPixbufEqual(originalPixbuf, highlightPixbuf));
> +
> +    // Requires http://webkit.org/b/77747 to be fixed
> +    // WebKitFindController* findController = webkit_web_view_get_find_controller(test->m_webView);
> +    // webkit_find_controller_search_finish(findController);
> +    // gtk_widget_show_now(GTK_WIDGET(test->m_webView));
> +    // webkit_web_view_execute_editing_command(test->m_webView, "Unselect");
> +
> +    // test->wait(1.0);
> +    // GdkPixbuf* unhighlightPixbuf = gdk_pixbuf_get_from_window(webViewGdkWindow, 0, 0, allocatedHeight, allocatedWidth);
> +    // g_assert(unhighlightPixbuf);
> +    // g_assert(gdkPixbufEqual(originalPixbuf, unhighlightPixbuf));
> +
> +    g_object_unref(originalPixbuf);

Waiting three seconds here is far too long, in my opinion. Does a 1/10th or 1/5th of a second work for these timeouts?

[Sergio Villar Senin]

Removing two bugs that depend on this one instead of blocking it. Adding 79252 to the list of dependencies, without it unit tests hang in debug builds.

[Sergio Villar Senin]

Created attachment 128749 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #52)
> Created an attachment (id=128749) [details]
> Patch

I finally managed to remove the nasty timeouts listening to webView's draw signal. Apart from that this patch fixes the minor issues detected by Martin and also replaces the guint type of find options by guint32, as find options in WKFindOptions.h are defined as uint32_t

[Carlos Garcia Campos]

Comment on attachment 128749 [details]
Patch

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

Looks great!

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:127
> +        g_value_set_object(value, findController->priv->webView);

Why don you use webkit_find_controller_get_web_view() in this case, like for the other properties?

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:150
> +    WEBKIT_FIND_CONTROLLER(object)->priv->~WebKitFindControllerPrivate();
> +    G_OBJECT_CLASS(webkit_find_controller_parent_class)->finalize(object);

We could call finish() here is it wasn't called to make sure the matches are always unhighlighted.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:379
> + * WebKitFindController::found-text and
> + * WebKitFindController::failed-to-find-text.

# is missing there #WebKitFindController::found-text and #WebKitFindController::failed-to-find-text

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:394
> + * This function will also highlight up to @max_match_count
> + * maches. Callers should call webkit_find_controller_search_finish()
> + * to unhighlight them.

Maybe, instead of adding this as an additional thing the method does, I would include it in the main explanation, Something like:

Looks for @search_text in the #WebKitWebView associated with @find_controller since the beginning of the document highlighting up to @max_match_count matches. 

Or something like that. And then: You should call webkit_find_controller_search_finish() to finish the current search operation. Isn't it obvious enough that the matches are unhighlighted when the search operation finishes? What do you think?

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:400
> +

This method starts a new search operation, so we could call finish() here too, to make sure any previous search operation is finished and matches are unhighlighted.

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:449
> + * WebKitFindController::counted-matches signal.

#WebKitFindController::counted-matches

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:466
> + * Finishes a find operation started by
> + * webkit_find_controller_search(). It will basically unhighlight
> + * every text match found.

You could say that this is typically called when the search ui is closed/hidden in the app.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:321
> +    GdkPixbuf* originalPixbuf = gdk_pixbuf_get_from_window(webViewGdkWindow, 0, 0, allocatedHeight, allocatedWidth);

You could use GRefPtr for the pixbuf

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:329
> +    GdkPixbuf* highlightPixbuf = gdk_pixbuf_get_from_window(webViewGdkWindow, 0, 0, allocatedHeight, allocatedWidth);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:341
> +    // Requires http://webkit.org/b/77747 to be fixed
> +    // WebKitFindController* findController = webkit_web_view_get_find_controller(test->m_webView);
> +    // webkit_find_controller_search_finish(findController);
> +    // gtk_widget_show_now(GTK_WIDGET(test->m_webView));
> +    // webkit_web_view_execute_editing_command(test->m_webView, "Unselect");
> +
> +    // test->waitUntilWidgetDrawn();
> +    // GdkPixbuf* unhighlightPixbuf = gdk_pixbuf_get_from_window(webViewGdkWindow, 0, 0, allocatedHeight, allocatedWidth);
> +    // g_assert(unhighlightPixbuf);
> +    // g_assert(gdkPixbufEqual(originalPixbuf, unhighlightPixbuf));

We use #if 0 in other tests, instead of commented code.

[Sergio Villar Senin]

(In reply to comment #54)
> > Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:150
> > +    WEBKIT_FIND_CONTROLLER(object)->priv->~WebKitFindControllerPrivate();
> > +    G_OBJECT_CLASS(webkit_find_controller_parent_class)->finalize(object);
> 
> We could call finish() here is it wasn't called to make sure the matches are always unhighlighted.

I don't understand this sentence. Could you rephrase it?
 
> Or something like that. And then: You should call webkit_find_controller_search_finish() to finish the current search operation. Isn't it obvious enough that the matches are unhighlighted when the search operation finishes? What do you think?

I think we should mention it anyway, I wouldn't say it's obvious.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:400
> > +
> 
> This method starts a new search operation, so we could call finish() here too, to make sure any previous search operation is finished and matches are unhighlighted.

I guess you mean that we should mention it in the docs right?

[Carlos Garcia Campos]

(In reply to comment #55)
> (In reply to comment #54)
> > > Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:150
> > > +    WEBKIT_FIND_CONTROLLER(object)->priv->~WebKitFindControllerPrivate();
> > > +    G_OBJECT_CLASS(webkit_find_controller_parent_class)->finalize(object);
> > 
> > We could call finish() here is it wasn't called to make sure the matches are always unhighlighted.
> 
> I don't understand this sentence. Could you rephrase it?

I meant we should make sure that the current search operation is finished when the object is finalized, but I had forgotten that the find controller object is alive until the view is finalized, so it doesn't make sense, because the view will be destroyed anyway. 

> > Or something like that. And then: You should call webkit_find_controller_search_finish() to finish the current search operation. Isn't it obvious enough that the matches are unhighlighted when the search operation finishes? What do you think?
> 
> I think we should mention it anyway, I wouldn't say it's obvious.
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:400
> > > +
> > 
> > This method starts a new search operation, so we could call finish() here too, to make sure any previous search operation is finished and matches are unhighlighted.
> 
> I guess you mean that we should mention it in the docs right?

No, I mean we should call webkit_find_controller_search_finish() or WKPageHideFindUI() directly if there's an ongoing search operation when a new one starts and the user hasn't explicitly called it already. Maybe we don't need it because the new operation already resets the old highlights, so I might be wrong here too.

[Sergio Villar Senin]

(In reply to comment #56)

> > > This method starts a new search operation, so we could call finish() here too, to make sure any previous search operation is finished and matches are unhighlighted.
> > 
> > I guess you mean that we should mention it in the docs right?
> 
> No, I mean we should call webkit_find_controller_search_finish() or WKPageHideFindUI() directly if there's an ongoing search operation when a new one starts and the user hasn't explicitly called it already. Maybe we don't need it because the new operation already resets the old highlights, so I might be wrong here too.

Exactly, the very first thing that is done by the FindController everytime a new search operation is started is unmarkAllTextMatches(), so there is no need to explicitly invoke a _finish().

[Philippe Normand]

Comment on attachment 128749 [details]
Patch

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

I second Carlos's comments. Apart from that it looks fine API-wise and the test as well.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:297
> +    int totalLenght = (pixbufHeight - 1) * pixbufRowstride \

typo: totalLength

[Carlos Garcia Campos]

If we all agree the patch is good and Martin already r+'ed the previous patch (the new one doesn't change the API nor the impl, just the unit tests), could some reviewer mark this r+, please?

[Martin Robinson]

Comment on attachment 128749 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFindController.cpp:71
> +    void waitUntilWidgetDrawn()

This might be more accurately called waitUntilWebViewDrawSignal.

[Sergio Villar Senin]

Committed r109222: <http://trac.webkit.org/changeset/109222>