[Webkit-unassigned] [Bug 163892] [GTK] Add function webkit_dom_element_get_bounding_client_rect

[bugzilla-daemon at webkit.org]

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

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #303456|review?                     |review+
              Flags|                            |

--- Comment #27 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 303456
  --> https://bugs.webkit.org/attachment.cgi?id=303456
Patch

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

Thanks for the patch, it looks great, I have only a few minor comments

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRectList.h:69
> + * Returns a #WebKitDOMClientRect object that @self contains.

Returns the #WebKitDOMClientRect object that @self contains at @index.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.h:526
> +**/

Since: 2.18

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.h:538
> +**/

Since: 2.18

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMClientRectTest.cpp:26
> +static bool testClientRectPosition(WebKitDOMClientRect* clientRect)

Why is this out of WebKitDOMClientRectTest class?

> 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);

We don't need to test this twice, g_object_get uses the public getters in the end

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMClientRectTest.cpp:51
> +    return true;

Why is this boolean if it always returns true?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMClientRectTest.cpp:74
> +        g_object_unref(clientRect);

Use GRefPtr

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMClientRectTest.cpp:96
> +        gulong clientRectsLength;
> +        g_object_get(clientRectList, "length", &clientRectsLength, nullptr);
> +        g_assert_cmpuint(clientRectsLength, ==, 1);

We don't need to check this twice either.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMClientRectTest.cpp:103
> +        g_object_unref(clientRectList);

Use GrefPtr

> 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);

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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170306/c948962b/attachment-0001.html>