Bug 69249 – [GTK][WEBKIT2] Add webkit_web_view_load_html and webkit_web_view_load_plain_text APIs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Nayan Kumar K]

Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text WebKit2-GTK+ API's.

[Nayan Kumar K]

Created attachment 109466 [details]
Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs.

[WebKit Review Bot]

Attachment 109466 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:97:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:97:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:233:  webkit_web_view_load_html_string is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:249:  webkit_web_view_load_plain_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 6 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Collabora GTK+ EWS bot]

Comment on attachment 109466 [details]
Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs.

Attachment 109466 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9926004

[Carlos Garcia Campos]

Comment on attachment 109466 [details]
Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs.

View in context: https://bugs.webkit.org/attachment.cgi?id=109466&action=review

> Source/WebKit2/GNUmakefile.am:1087
> -	Programs/unittests/webkit2/testloading
> +	Programs/unittests/webkit2/testloading \
> +	Programs/unittests/webkit2/testhtmlstringloading \
> +	Programs/unittests/webkit2/testplaintextloading

I don't think we need new tests, just add two new test cases to the existing testloading.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:277
> +    g_return_if_fail(baseUri);

The documentation says NULL is allowed for baseUri, about:blank is used.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:280
> +    WKPageLoadHTMLString(toAPI(page), WKStringCreateWithUTF8CString(content), WKURLCreateWithUTF8CString(baseUri));

You are leaking the WKURL and WKString, see bug https://bugs.webkit.org/show_bug.cgi?id=69247

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:296
> +    WKPageLoadPlainTextString(toAPI(page), WKStringCreateWithUTF8CString(plainText));

And here the WKString

[Nayan Kumar K]

Created attachment 109473 [details]
Load HTML String and Plain text API

Incorporated review comments.

[WebKit Review Bot]

Attachment 109473 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:97:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:97:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:233:  webkit_web_view_load_html_string is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:252:  webkit_web_view_load_plain_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 109473 [details]
Load HTML String and Plain text API

View in context: https://bugs.webkit.org/attachment.cgi?id=109473&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:277
> +void webkit_web_view_load_html_string(WebKitWebView* webView, const gchar* content, const gchar* baseUri)
> +{
> +    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));
> +

baseUri can be NULL, but you should check content is not NULL. I didn't notice it was missing in previous patch, sorry.

> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:232
> +typedef struct {
> +    WebKitWebView *webView;
> +    GMainLoop *loop;
> +    gboolean hasBeenFinished;
> +} HtmlStringLoadingFixture;

You can use the same Fixture for new tests.

> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:234
> +static void htmlStringLoadingNotifyTitleCb(WebKitWebView* webView, GParamSpec* pSpec, HtmlStringLoadingFixture* fixture)

Tests are C, not C++, so it should be WebKitWebView *webView, GParamSpec *pSpec, HtmlStringLoadingFixture *fixture)

> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:238
> +    const gchar* title = webkit_web_view_get_title(webView);

* is missplaced here too.

> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:278
> +    g_signal_connect(client, "load-failed", G_CALLBACK(htmlStringLoadStatusLoadFailed), fixture);

You should check provisional-load-failed too.

[Nayan Kumar K]

Comment on attachment 109473 [details]
Load HTML String and Plain text API

View in context: https://bugs.webkit.org/attachment.cgi?id=109473&action=review

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:277
>> +
> 
> baseUri can be NULL, but you should check content is not NULL. I didn't notice it was missing in previous patch, sorry.

Done. Thank you for pointing it out.

>> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:234
>> +static void htmlStringLoadingNotifyTitleCb(WebKitWebView* webView, GParamSpec* pSpec, HtmlStringLoadingFixture* fixture)
> 
> Tests are C, not C++, so it should be WebKitWebView *webView, GParamSpec *pSpec, HtmlStringLoadingFixture *fixture)

Done.

>> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:238
>> +    const gchar* title = webkit_web_view_get_title(webView);
> 
> * is missplaced here too.

Done.

>> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:278
>> +    g_signal_connect(client, "load-failed", G_CALLBACK(htmlStringLoadStatusLoadFailed), fixture);
> 
> You should check provisional-load-failed too.

Done.

[Nayan Kumar K]

Created attachment 109490 [details]
Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs.

Incorporated review comments.

[WebKit Review Bot]

Attachment 109490 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:97:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:97:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:233:  webkit_web_view_load_html_string is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:253:  webkit_web_view_load_plain_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 109490 [details]
Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs.

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.

[Nayan Kumar K]

Comment on attachment 109490 [details]
Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs.

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.

[Nayan Kumar K]

Created attachment 109608 [details]
Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs.

[WebKit Review Bot]

Attachment 109608 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:97:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:97:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:235:  webkit_web_view_load_html_string is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:256:  webkit_web_view_load_plain_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 109608 [details]
Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs.

Attachment 109608 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9930767

[Martin Robinson]

Comment on attachment 109608 [details]
Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs.

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.

[Carlos Garcia Campos]

(In reply to comment #16)
> > 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.

Yes, there's WKRetainPtr, I realized yesterday, I'm already using it in the bf list patch:

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

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

No, we don't need a new fiture for every test, we can just use the loading one, see the load_alternate_html() patch:

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

[Nayan Kumar K]

Comment on attachment 109608 [details]
Add webkit_web_view_load_html_string and webkit_web_view_load_plain_text APIs.

View in context: https://bugs.webkit.org/attachment.cgi?id=109608&action=review

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

Done.

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

Sure. I will rename this function name.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:286
>> +    WKRelease(baseUriRef);
> 
> Is there no smart pointer for WK types? If there is we need to use it.

There is WKRetainPtr, as mentioned by Carlos. I will use it here.

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

Ok.

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

As we don't have the API to get the loaded content, I thought of asserting on expected text for now. But as you said, for now I will keep this test very simple, just asserting if load is not finished. Once we have the API to get content loaded, I will modify this test,

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

Sorry, I didn't quite understand what you meant by webkittests file? Should I move this test to testwebview.c file?

[Nayan Kumar K]

Created attachment 109962 [details]
Load HTML String and Plain text API

Updated the patch as per review comments.

Please note that tests included in this patch are same as 'webkit_web_view_load_alternate_html'. Since there aren't any more _load API's, I think its OK to leave the tests the way it is.

[WebKit Review Bot]

Comment on attachment 109962 [details]
Load HTML String and Plain text API

Rejecting attachment 109962 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Attachment 109962 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:97:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:97:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:263:  webkit_web_view_load_html is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:282:  webkit_web_view_load_plain_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 109962 [details]
Load HTML String and Plain text API

View in context: https://bugs.webkit.org/attachment.cgi?id=109962&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:256
> + * @html: The HTML string to load

Use content instead of html for consistency with webkit_web_view_load_alternate_html.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:262
> + * The mime type of the document will be "text/html".
> + */

Please add:

 * You can monitor the status of the load operation using the
 * #WebKitWebLoaderClient of @web_view. See webkit_web_view_get_loader_client().

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:264
> +void webkit_web_view_load_html(WebKitWebView* webView, const gchar* html, const gchar* baseUri)
> +{

Use baseURI instead of baseUri.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:281
> + * Load the specified plain text string into @web_view. The mime type of
> + * document will be "text/plain".
> + */

Please add:

 * You can monitor the status of the load operation using the
 * #WebKitWebLoaderClient of @web_view. See webkit_web_view_get_loader_client().

[Nayan Kumar K]

Created attachment 110117 [details]
Load HTML String and Plain text API

[Carlos Garcia Campos]

Comment on attachment 110117 [details]
Load HTML String and Plain text API

View in context: https://bugs.webkit.org/attachment.cgi?id=110117&action=review

Patch looks good to me, I think you just need to update it to current git master and use the new unit tests stuff. Look at how load_alternate_content is tested in TestWebKitWebLoaderClient.cpp and add somethihng similar for html and plain_text. Thanks!

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

add an introspection tag to indicate that base_uri can be null:

@base_uri: (allo-none): the base URI for relative locations or %NULL.

and move the comment that when null it defaults to about:blank below to the main doc body.

[Nayan Kumar K]

Created attachment 111751 [details]
Add webkit_web_view_load_html and webkit_web_view_load_plain_text API's.

[Carlos Garcia Campos]

Comment on attachment 111751 [details]
Add webkit_web_view_load_html and webkit_web_view_load_plain_text API's.

View in context: https://bugs.webkit.org/attachment.cgi?id=111751&action=review

Patch looks good to me.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:268
> + * When @base_uri is NULL, it defaults to "about:blank". The mime type 

Use %NULL here too.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:76
> +    webkit_web_view_load_plain_text(test->m_webView,
> +                                    "Hello WebKit-GTK+");

I think this could be better as a single line

[Nayan Kumar K]

Created attachment 111770 [details]
Load HTML String and Plain text API

[Martin Robinson]

Comment on attachment 111770 [details]
Load HTML String and Plain text API

View in context: https://bugs.webkit.org/attachment.cgi?id=111770&action=review

> Source/WebKit2/ChangeLog:8
> +        This patch adds support for 2 more load api's, namely

Nit: api's -> APIs

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:72
> +static void testLoadHtml(LoadTrackingTest* test, gconstpointer)
> +{
> +    webkit_web_view_load_html(test->m_webView, "<html><body>Hello WebKit-GTK+</body></html>", 0);
> +    test->waitUntilLoadFinished();
> +    assertNormalLoadHappenedAndClearEvents(test->m_loadEvents);
> +}
> +
> +static void testPlainText(LoadTrackingTest* test, gconstpointer)

These tests should be in TestWebKitWebView. The other WebKitWebView tests should be moving there shortly as well.

[Nayan Kumar K]

Created attachment 112972 [details]
New API addition.

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 112972 [details]
New API addition.

Patch looks good to me, the only thing missing is adding new symbols to UIProcess/API/gtk/docs/webkit2gtk-sections.txt, see http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 112972 [details]
New API addition.

This looks fine to me, but it's missing the sections of the gtkdoc, as Carlos said.

[Nayan Kumar K]

Created attachment 113455 [details]
New API addition.

[Nayan Kumar K]

Created attachment 113456 [details]
New API addition.

Few changes made in this patch,
a). Re-based the patch to latest trunk.
b). Restructured the unit tests as per the current design. Please note that TestWebKitWebLoaderClient.cpp file (under WebKitWebView suite name), to be consistent with rest of the similar tests (e.g. testAlternateContent).
c). Added new API's in 'webkit2gtk-sections.txt' file.

[Carlos Garcia Campos]

Comment on attachment 113456 [details]
New API addition.

LGTM

[Philippe Normand]

Comment on attachment 113456 [details]
New API addition.

Ok, let's do this! Thanks for the patch Nayan.

[WebKit Review Bot]

Comment on attachment 113456 [details]
New API addition.

Clearing flags on attachment: 113456

Committed r99176: <http://trac.webkit.org/changeset/99176>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.