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

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted 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 126984: Patch
https://bugs.webkit.org/attachment.cgi?id=126984&action=review

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