[webkit-reviews] review denied: [Bug 69249] [GTK][WEBKIT2] Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs : [Attachment 109608] Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs.

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Nayan Kumar K
<nayankk at motorola.com>'s request for review:
Bug 69249: [GTK][WEBKIT2] Add webkit_web_view_load_html_string and
webkit_web_view_load_plain_text APIs
https://bugs.webkit.org/show_bug.cgi?id=69249

Attachment 109608: Add webkit_web_view_load_html_string and
webkit_web_view_load_plain_text APIs.
https://bugs.webkit.org/attachment.cgi?id=109608&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109608&action=review


Looking good, but have some nits.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:274
> + * Mime type of @html_string is assumed to be "text/html".

Perhaps "The mime type of the document will be "text/html"."

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:276
> +void webkit_web_view_load_html_string(WebKitWebView* webView, const gchar*
htmlString, const gchar* baseUri)

I agree with Carlos that this should be called webkit_web_view_load_html.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:286
> +    WebPageProxy* page =
webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(webView));
> +    WKStringRef htmlStringRef = WKStringCreateWithUTF8CString(htmlString);
> +    WKURLRef baseUriRef = WKURLCreateWithUTF8CString(baseUri);
> +    WKPageLoadHTMLString(toAPI(page), htmlStringRef, baseUriRef);
> +    WKRelease(htmlStringRef);
> +    WKRelease(baseUriRef);

Is there no smart pointer for WK types? If there is we need to use it.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:292
> + * @plain_text: The plain text to load

A plain text string to load?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:295
> + * Load the specified plain text into @web_view. Plain text is assumed to
> + * have the mime-type "text/plain".

Same style of sentence for the mime type here. Please use "mime type"
everywhere and not "mime-type" in some places.

> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:28
> +#define HTML_TITLE "Hello WebKit GTK+!"

I don't think you need to test the title when loading a string.

> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:249
> +static void htmlStringLoadingNotifyTitleCb(WebKitWebView *webView,
GParamSpec *pSpec, WebLoadingFixture *fixture)
> +{
> +    g_assert(!fixture->hasBeenFinished);
> +
> +    const gchar *title = webkit_web_view_get_title(webView);
> +    g_assert_cmpstr(title, ==, HTML_TITLE);
> +}
> +
> +static void htmlStringLoadingFixtureSetup(WebLoadingFixture *fixture,
gconstpointer data)
> +{
> +    fixture->webView = WEBKIT_WEB_VIEW(webkit_web_view_new());
> +    fixture->loop = g_main_loop_new(NULL, TRUE);
> +    g_object_ref_sink(fixture->webView);
> +    fixture->hasBeenFinished = FALSE;
> +    g_signal_connect(fixture->webView, "notify::title",
G_CALLBACK(htmlStringLoadingNotifyTitleCb), fixture);
> +}
> +
> +static gboolean htmlStringLoadStatusLoadFinished(WebKitWebLoaderClient
*client, WebLoadingFixture *fixture)
> +{
> +    g_assert(!fixture->hasBeenFinished);
> +    fixture->hasBeenFinished = TRUE;
> +
> +    g_main_loop_quit(fixture->loop);

I guess it's okay to structure the test like this for now, but when we add an
API to get the WebView contents, we should restructure this test to simply load
the page and assert that the contents are equal. These tests should be in the
webkittests file since they are unit tests for the WebView.