[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 #109490|review?                     |review-
               Flag|                            |




--- Comment #11 from Martin Robinson <mrobinson at webkit.org>  2011-10-03 13:27:58 PST ---
(From update of attachment 109490)
View in context: https://bugs.webkit.org/attachment.cgi?id=109490&action=review

> Source/WebKit2/ChangeLog:4
> +        Add webkit_web_view_load_html_string and
> +        webkit_web_view_load_plain_text APIs.

This should be the bug title exactly and one line.  Are you using prepare-ChangeLog to generate your changelogs?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:267
> + * @content: HTML string to be loaded

Should be "the HTML string to load" to avoid the passive voice in this case.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:268
> + * @base_uri: the base URI for relative locations. If %NULL, defaults to about:blank

Doesn't this need to match the parameter name here or in the header? We really need to get the gtkdoc rules going so that we can verify that new documentation does not produce warnings / broken documentation.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:272
> + * Requests loading of the given @content with the specified @base_uri. 
> + * The @base_uri represents the @content that is loaded through this API.
> + * As such, it is used to resolve any relative URLs.

I think this can be more active. Suggestion:

Load the given @content string with the specified @base_uri. Relative URLs in the @content will be resolved against @base_uri.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:290
> + * @plain_text: Text to be displayed

You use plan_text here, but not html or html_string above. I suggest making the parameter names consistent.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:292
> + * Loads the specified text into @web_view.

You should probably talk a little bit more about why this is different than webkit_web_view_load_html_string. Namely the mime type of the document is different.

> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:30
> +#define PLAIN_TEXT "Welcome to WebKit GTK+!"

Please make this a static const char* and in the local scope of where you use it.

> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:262
> +    g_signal_connect(client, "load-finished", G_CALLBACK(htmlStringLoadStatusLoadFinished), fixture);
> +    g_signal_connect(client, "load-failed", G_CALLBACK(loadStatusLoadFailed), fixture);
> +    g_signal_connect(client, "provisional-load-failed", G_CALLBACK(loadStatusProvisionalLoadFailed), fixture);
> +

Instead of doing the same thing that the other loader tests does, it would be better to simply connect to load-finished and assert the contents are what you expect.

> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:294
> +    g_signal_connect(client, "load-finished", G_CALLBACK(plainTextLoadStatusLoadFinished), fixture);
> +    g_signal_connect(client, "load-failed", G_CALLBACK(loadStatusLoadFailed), fixture);
> +    g_signal_connect(client, "provisional-load-failed", G_CALLBACK(loadStatusProvisionalLoadFailed), fixture);
> +

Ditto.

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