[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





--- Comment #12 from Nayan Kumar K <nayankk at motorola.com>  2011-10-04 05:14:02 PST ---
(From update of attachment 109490)
View in context: https://bugs.webkit.org/attachment.cgi?id=109490&action=review

>> Source/WebKit2/ChangeLog:4
>> +        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?

Done. Since the bus title is big, I thought of wrapping it to next line. Sorry about that.

>> 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.

Done.

>> 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.

Submitted the gtk-doc generation patch at https://bugs.webkit.org/show_bug.cgi?id=69325. gtk-doc matches the parameter name with the one mentioned in header file.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:272
>> + * 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.

Done.

>> 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.

Done. Changed the above variable to html_string. It was done to maintain the consistency between WebKit1 and WebKit2 GTK API.

>> 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.

Done.

>> 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.

Done.

>> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:262
>> +
> 
> 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.

Removed connecting to 'load-failed' and 'provisional-load-failed'. Idea of this test was to verify whether document we requested to load is actually loaded, without any errors.

We can not assert on the contents what we expect, as there is no way to know the actual content we receive. For load_html_string string test, we can at least assert on what title we had asked to load, and same is implemented in htmlStringLoadingNotifyTitleCb.

>> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:294
>> +
> 
> Ditto.

Removed connecting to 'load-failed' and 'provisional-load-failed' signals. Now this test basically just checks whether plain text we asked to load gets loaded successfully. As explained earlier, we can not have the logic of asserting on content we expect, as there is no way to know the content we receive yet.

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