<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<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#c8">Comment # 8</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:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=302841&action=diff" name="attach_302841" title="Patch">attachment 302841</a> <a href="attachment.cgi?id=302841&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=302841&action=review">https://bugs.webkit.org/attachment.cgi?id=302841&action=review</a>
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.
<span class="quote">> Source/WebKit2/PlatformGTK.cmake:362
> + WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp
> WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMCharacterData.cpp</span >
These should be reversed (alphabetized).
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:2
> + * This file is part of the WebKit open source project.</span >
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.
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:23
> +#include <WebCore/CSSImportRule.h></span >
Please familiarize yourself with the rules for #include order and adjust the list accordingly.:
<a href="https://webkit.org/code-style-guidelines/#include-statements">https://webkit.org/code-style-guidelines/#include-statements</a>
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:46
> + return 0;</span >
Use nullptr
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:56
> + return request ? static_cast<WebCore::ClientRect*>(WEBKIT_DOM_OBJECT(request)->coreObject) : 0;</span >
nullptr
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:118
> +static GObject* webkit_dom_client_rect_constructor(GType type, guint constructPropertiesCount, GObjectConstructParam* constructProperties)</span >
I think you can use constructed for this instead of constructor. It would be a lot simpler.
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:143
> + "read-only gfloat ClientRect:top",</span >
Ahaha, not a very good property description. :P I guess it's fine, though, since it's the style used by existing DOM objects.
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:1257
> + g_return_val_if_fail(WEBKIT_DOM_IS_ELEMENT(self), 0);</span >
nullptr
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:1266
> + g_return_val_if_fail(WEBKIT_DOM_IS_ELEMENT(self), 0);</span >
nullptr
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:1270
> + size_t clientRectsLength = clientRects.get()->length();</span >
GList* clientRectList
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.h:532
> + * Returns: (element-type WebKitDOMClientRect) (transfer full): A #GList</span >
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)?
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/webkitdomautocleanups.h:124
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC (WebKitDOMClientRect, g_object_unref)</span >
Ah good, it's easy to forget to do this....</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>