[Webkit-unassigned] [Bug 69249] [GTK][WEBKIT2] Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=69249


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #109608|review?                     |review-
               Flag|                            |




--- Comment #16 from Martin Robinson <mrobinson at webkit.org>  2011-10-04 16:15:19 PST ---
(From update of attachment 109608)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.