[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

--- Comment #8 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 302841
  --> https://bugs.webkit.org/attachment.cgi?id=302841
Patch

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

OK, so this was a lot more work than I was expecting... but you seem to have handled it all fine. It looks good to me, with just a few minor complaints. We need to wait for Carlos Garcia to review it too.

> Source/WebKit2/PlatformGTK.cmake:362
> +    WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp
>      WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMCharacterData.cpp

These should be reversed (alphabetized).

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:2
> + *  This file is part of the WebKit open source project.

So there should be a copyright notice here to complement the copyright license below. Unless for some reason you really don't want your name going into the source code.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:23
> +#include <WebCore/CSSImportRule.h>

Please familiarize yourself with the rules for #include order and adjust the list accordingly.:

https://webkit.org/code-style-guidelines/#include-statements

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:46
> +        return 0;

Use nullptr

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:56
> +    return request ? static_cast<WebCore::ClientRect*>(WEBKIT_DOM_OBJECT(request)->coreObject) : 0;

nullptr

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:118
> +static GObject* webkit_dom_client_rect_constructor(GType type, guint constructPropertiesCount, GObjectConstructParam* constructProperties)

I think you can use constructed for this instead of constructor. It would be a lot simpler.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:143
> +            "read-only gfloat ClientRect:top",

Ahaha, not a very good property description. :P  I guess it's fine, though, since it's the style used by existing DOM objects.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:1257
> +    g_return_val_if_fail(WEBKIT_DOM_IS_ELEMENT(self), 0);

nullptr

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:1266
> +    g_return_val_if_fail(WEBKIT_DOM_IS_ELEMENT(self), 0);

nullptr

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:1270
> +    size_t clientRectsLength = clientRects.get()->length();

GList* clientRectList

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.h:532
> + * Returns: (element-type WebKitDOMClientRect) (transfer full): A #GList

Is it really transfer full, or is it actually transfer container (where the GList itself is owned by the caller, but the elements are unowned)? It's not clear to me from the implementation. i.e., do you free the list with g_list_free(list) or with g_list_free_full(list, g_object_unref)?

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/webkitdomautocleanups.h:124
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC (WebKitDOMClientRect, g_object_unref)

Ah good, it's easy to forget to do this....

-- 
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/20170227/7ad49429/attachment.html>