[webkit-reviews] review denied: [Bug 76070] [GTK] [WK2] Add Find API : [Attachment 122900] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 76070: [GTK] [WK2] Add Find API
https://bugs.webkit.org/show_bug.cgi?id=76070

Attachment 122900: Patch
https://bugs.webkit.org/attachment.cgi?id=122900&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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!