<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add function webkit_dom_element_get_bounding_client_rect"
href="https://bugs.webkit.org/show_bug.cgi?id=163892">bug 163892</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #303456 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add function webkit_dom_element_get_bounding_client_rect"
href="https://bugs.webkit.org/show_bug.cgi?id=163892#c27">Comment # 27</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add function webkit_dom_element_get_bounding_client_rect"
href="https://bugs.webkit.org/show_bug.cgi?id=163892">bug 163892</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=303456&action=diff" name="attach_303456" title="Patch">attachment 303456</a> <a href="attachment.cgi?id=303456&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=303456&action=review">https://bugs.webkit.org/attachment.cgi?id=303456&action=review</a>
Thanks for the patch, it looks great, I have only a few minor comments
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRectList.h:69
> + * Returns a #WebKitDOMClientRect object that @self contains.</span >
Returns the #WebKitDOMClientRect object that @self contains at @index.
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.h:526
> +**/</span >
Since: 2.18
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.h:538
> +**/</span >
Since: 2.18
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMClientRectTest.cpp:26
> +static bool testClientRectPosition(WebKitDOMClientRect* clientRect)</span >
Why is this out of WebKitDOMClientRectTest class?
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMClientRectTest.cpp:49
> + gfloat top, right, bottom, left, width, height;
> + g_object_get(clientRect,
> + "top", &top,
> + "right", &right,
> + "bottom", &bottom,
> + "left", &left,
> + "width", &width,
> + "height", &height, nullptr);
> + g_assert_cmpfloat(top, ==, -25);
> + g_assert_cmpfloat(right, ==, 50);
> + g_assert_cmpfloat(bottom, ==, 175);
> + g_assert_cmpfloat(left, ==, -50);
> + g_assert_cmpfloat(width, ==, 100);
> + g_assert_cmpfloat(height, ==, 200);</span >
We don't need to test this twice, g_object_get uses the public getters in the end
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMClientRectTest.cpp:51
> + return true;</span >
Why is this boolean if it always returns true?
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMClientRectTest.cpp:74
> + g_object_unref(clientRect);</span >
Use GRefPtr
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMClientRectTest.cpp:96
> + gulong clientRectsLength;
> + g_object_get(clientRectList, "length", &clientRectsLength, nullptr);
> + g_assert_cmpuint(clientRectsLength, ==, 1);</span >
We don't need to check this twice either.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMClientRectTest.cpp:103
> + g_object_unref(clientRectList);</span >
Use GrefPtr
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMClientRectTest.cpp:125
> + WebKitDOMClientRect* item1 = webkit_dom_client_rect_list_item(clientRectList, 0);
> + WebKitDOMClientRect* item2 = webkit_dom_client_rect_list_item(clientRectList, 0);
> + g_assert(item1 == item2);</span >
You can do this check in the previous test case, I don't think we need an entire test casee only for this, add g_assert(webkit_dom_client_rect_list_item(clientRectList, 0) == webkit_dom_client_rect_list_item(clientRectList, 0)); to the previous test.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>