[webkit-reviews] review denied: [Bug 118430] [GTK] Migrate WebKitDOMDocument unit tests to WebKit2 : [Attachment 214590] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Dec 23 01:54:25 PST 2013
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 214590: Patch
https://bugs.webkit.org/attachment.cgi?id=214590&action=review
------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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.
More information about the webkit-reviews
mailing list