[webkit-reviews] review denied: [Bug 69249] [GTK][WEBKIT2] Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs : [Attachment 109490] 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 109490: Add webkit_web_view_load_html_string and
webkit_web_view_load_plain_text APIs.
https://bugs.webkit.org/attachment.cgi?id=109490&action=review

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