[webkit-reviews] review denied: [Bug 111288] [GTK][WK2] Add webkit_web_page_get_uri to WebKit2 GTK+ API : [Attachment 191209] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 111288: [GTK][WK2] Add webkit_web_page_get_uri to WebKit2 GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=111288

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

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=191209&action=review


> Source/WebKit2/UIProcess/API/gtk/tests/TestLoaderClient.cpp:297
> +	   const char* uri;
> +	   g_variant_get(result, "(&s)", &uri);
> +	   g_assert_cmpstr(uri, ==, webkit_web_view_get_uri(test->m_webView));
> +	   test->m_URIs.append(uri);

I've noticed that this test is flaky, it depends on whether the dbus message is
received after or before the wk message. I think you could connect to
WebKitWebView::notify::uri and fill another Vector. And in the test compare the
vectors removing the comparison here.

> Source/WebKit2/UIProcess/API/gtk/tests/TestLoaderClient.cpp:329
> +    g_assert_cmpstr(test->m_URIs[0].data(), ==,
kServer->getURIForPath("/redirect").data());
> +    g_assert_cmpstr(test->m_URIs[1].data(), ==,
kServer->getURIForPath("/normal").data());

Also noticed a problem here, kServer->getURIForPath returns a temporary
CString, that can't be used with g_assert_cmpstr because it's macro, not a
function. See bug https://bugs.webkit.org/show_bug.cgi?id=111346. Use
ASSERT_CMP_CSTRING when the patch in that bug lands.