Bug 118430 – [GTK] Migrate WebKitDOMDocument unit tests to WebKit2

• Status: ASSIGNED
• Component: Tools / Tests

[Carlos Garcia Campos]

ssia

[Enrique Ocaña]

I'm working on this.

[Enrique Ocaña]

Created attachment 213360 [details]
Patch

[Enrique Ocaña]

Take into account the comment in the patch:

This patch doesn't migrate test_dom_document_garbage_collection() because it requires API to load a new document in WebProcess (the equivalent of webkit_web_view_load_string()) and it's not currently available.

A new bug should be opened to migrate the pending test.

[Carlos Garcia Campos]

Comment on attachment 213360 [details]
Patch

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

Looks great, there are just a few style issues. Instead of copying the tests as they are in WebKit1, we can make some improvements:
 - Use the gobject macros to check DOM objects
 - Use GRefPtr/GOwnPtr when appropriate
 - Do not use C style casts
 - Clean up the code when appropriate

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:53
> +        gchar* title = webkit_dom_document_get_title(document);

Use GOwnPtr<char> instead of gchar*

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:58
> +        title = webkit_dom_document_get_title(document);

And title.set() here

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:72
> +        WebKitDOMNodeList* list = webkit_dom_document_get_elements_by_tag_name(document, "li");

Use GRefPtr<WebKitDOMNodeList> here.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:73
> +        g_assert(list);

g_assert(WEBKIT_DOM_IS_NODE_LIST(list)); Use always the GObject macros to check also that the returned pointer is the right glib type.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:79
> +        guint i;
> +
> +        for (i = 0; i < length; i++) {

for (unsigned i = 0; ....

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:81
> +            g_assert(item);

Use the gobject macro here too.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:83
> +            g_assert(element);

Ditto, but we don't need a new variable for that, use item instead:

g_assert(WEBKIT_DOM_IS_ELEMENT(item));

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:84
> +            g_assert_cmpstr(webkit_dom_element_get_tag_name(element), ==, "LI");

And here you can use item directly too:

g_assert_cmpstr(webkit_dom_element_get_tag_name(WEBKIT_DOM_ELEMENT(element)), ==, "LI");

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:85
> +            WebKitDOMHTMLElement* htmlElement = (WebKitDOMHTMLElement*)element;

Same here, we don't need this, we can simply cast item when needed, using the gobject macros instead of C style casts.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:86
> +            char* n = g_strdup_printf("%d", i+1);

Use GOwnPtr<char> and a more descriptive name for the variable instead of "n", expectedInnerText for example.
i+1 -> i + 1

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:96
> +    bool testGetElementsByClassName(WebKitWebExtension* extension, GVariant* args)

Same comments apply to this test too.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:138
> +        WebKitDOMHTMLElement* htmlElement = (WebKitDOMHTMLElement*)element;
> +        g_assert_cmpstr(webkit_dom_html_element_get_inner_text(htmlElement), ==, "first");

Same here, use gobject macros and don't cache this variable using a C style cast. Use a gobject cast directly when using it.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:143
> +    bool testGetLinks(WebKitWebExtension* extension, GVariant* args)

Same comments here.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:211
> +        int i = 0;

This could be unsigned

[Enrique Ocaña]

Created attachment 214590 [details]
Patch

[WebKit Commit 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 214590 [details]
Patch

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

Thanks for the patch. It looks good in general, but you are overusing and misusing the smart pointers everywhere. In general, dom api returning objects are transfer none, and you don't need to take a reference nor release it, and methods returning a string are transfter full, and you need to free the returned string. In any case, you can see what every method return in the documentation http://webkitgtk.org/reference/webkitdomgtk/2.3.3

> Source/WebKit2/ChangeLog:8
> +        This patch doesn't migrate test_dom_document_garbage_collection() because
> +        it requires API to load a new document in WebProcess (the equivalent of
> +        webkit_web_view_load_string()) and it's not currently available.

Please move the description after the Reviewed by line

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:51
> +        GRefPtr<WebKitWebPage> page(webkit_web_extension_get_page(extension, webPageFromArgs(args)));

webkit_web_extension_get_page is trabsfer none, you don't need to use GRefPtr.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:53
> +        GRefPtr<WebKitDOMDocument> document(webkit_web_page_get_dom_document(page.get()));

Same here

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:57
> +        g_assert(title.get());
> +        g_assert_cmpstr(title.get(), ==, "This is the title");

This is redundant, if title is NULL it's not "This is the title" so we only need the second assert.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:61
> +        g_assert(title.get());
> +        g_assert_cmpstr(title.get(), ==, "This is the second title");

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:68
> +        GRefPtr<WebKitWebPage> page(webkit_web_extension_get_page(extension, webPageFromArgs(args)));

You don't need to use GRefPtr

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:70
> +        GRefPtr<WebKitDOMDocument> document(webkit_web_page_get_dom_document(page.get()));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:72
> +        GRefPtr<WebKitDOMNodeList> list(webkit_dom_document_get_elements_by_tag_name(document.get(), "li"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:78
> +            GRefPtr<WebKitDOMNode> item(webkit_dom_node_list_item(list.get(), i));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:80
> +            g_assert(WEBKIT_DOM_IS_NODE(item.get()));
> +            g_assert(WEBKIT_DOM_IS_ELEMENT(item.get()));

WebKitDOMElement derives from WebKitDOMNode, so you only need to check whether it's element. You should probably check WEBKIT_DOM_IS_HTML_ELEMENT instead.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:81
> +            g_assert_cmpstr(webkit_dom_element_get_tag_name(WEBKIT_DOM_ELEMENT(item.get())), ==, "LI");

You are leaking the tag name here, webkit_dom_element_get_tag_name() returns a newly allocated string, you should use GOwnPtr

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:82
> +            GOwnPtr<char> expectedInnerText(g_strdup_printf("%d", i + 1));

Use %u instead of %d since i is unsigned.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:83
> +            g_assert_cmpstr(webkit_dom_html_element_get_inner_text(WEBKIT_DOM_HTML_ELEMENT(item.get())), ==, expectedInnerText.get());

You are leaking the text here, you should use GOwnPtr

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:91
> +        GRefPtr<WebKitWebPage> page(webkit_web_extension_get_page(extension, webPageFromArgs(args)));

No need for GrefPtr in this case.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:93
> +        GRefPtr<WebKitDOMDocument> document(webkit_web_page_get_dom_document(page.get()));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:95
> +        GRefPtr<WebKitDOMNodeList> list(webkit_dom_document_get_elements_by_class_name(document.get(), "test"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:101
> +            GRefPtr<WebKitDOMNode> item(webkit_dom_node_list_item(list.get(), i));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:103
> +            GRefPtr<WebKitDOMElement> element(WEBKIT_DOM_ELEMENT(item.get()));

You don't need to use GRefPtr just for a cast.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:104
> +            g_assert(WEBKIT_DOM_IS_ELEMENT(element.get()));

I think it should be enough to check this, all other checks are redundant, because a WebKitDOMElement is always a WebKitDOMNode

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:105
> +            g_assert_cmpstr(webkit_dom_element_get_tag_name(element.get()), ==, "DIV");

You are leaking the tag name here, you should use GOwnPtr.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:113
> +        GRefPtr<WebKitWebPage> page(webkit_web_extension_get_page(extension, webPageFromArgs(args)));

No need to use GRefPtr here.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:115
> +        GRefPtr<WebKitDOMDocument> document(webkit_web_page_get_dom_document(page.get()));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:117
> +        GRefPtr<WebKitDOMElement> element(webkit_dom_document_get_element_by_id(document.get(), "testok"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:123
> +        /* The DOM spec says the return value is undefined when there's
> +         * more than one element with the same id; in our case the first
> +         * one will be returned */

Don not use C style comments. The lines could be longer, maybe even a single line.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:126
> +        GRefPtr<WebKitDOMHTMLElement> htmlElement(WEBKIT_DOM_HTML_ELEMENT(element.get()));

You don't need to use GrefPtr just for a cast

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:127
> +        g_assert_cmpstr(webkit_dom_html_element_get_inner_text(htmlElement.get()), ==, "first");

You are leaking the text here,. use GOwnPtr

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDocumentTest.cpp:134
> +        GRefPtr<WebKitWebPage> page(webkit_web_extension_get_page(extension, webPageFromArgs(args)));

Ok, I'm not going to repeat myself again :-) You have the same issues in all the tests.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDOMDocument.cpp:110
> +    static const char* testHTML = "<html><head><title></title></head><body><div>First div</div><div>Second div</div></body></html>";
> +    test->loadHtml(testHTML, 0);
> +    test->waitUntilLoadFinished();
> +
> +    GVariantBuilder builder;
> +    g_variant_builder_init(&builder, G_VARIANT_TYPE_VARDICT);
> +    g_variant_builder_add(&builder, "{sv}", "pageID", g_variant_new_uint64(webkit_web_view_get_page_id(test->m_webView)));
> +    g_assert(testRunner->runTest("WebKitDOMDocument", "document-evaluate", g_variant_builder_end(&builder)));

This code is mostly duplicated in all the test cases, the only thing that changes is the html and the test name. You could add a helper function webkitDOMDocumentTestRun(WebViewTest* test, const char* html, const char* testName) for example.

[Enrique Ocaña]

Created attachment 222122 [details]
Patch

[Michael Catanzaro]

Comment on attachment 222122 [details]
Patch

This patch includes changes to old Autotools files. Removing from request queue.