[webkit-reviews] review denied: [Bug 118430] [GTK] Migrate WebKitDOMDocument unit tests to WebKit2 : [Attachment 213360] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Enrique Ocaña
<eocanha at igalia.com>'s request for review:
Bug 118430: [GTK] Migrate WebKitDOMDocument unit tests to WebKit2
https://bugs.webkit.org/show_bug.cgi?id=118430

Attachment 213360: Patch
https://bugs.webkit.org/attachment.cgi?id=213360&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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