<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 #302921 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#c14">Comment # 14</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=302921&action=diff" name="attach_302921" title="Patch">attachment 302921</a> <a href="attachment.cgi?id=302921&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=302921&action=review">https://bugs.webkit.org/attachment.cgi?id=302921&action=review</a>
Thanks for the patch! It's true that we don't have API tests to cover all the DOM bindings, mainly because it used to be autogenerated and we didn't even know when new API was added. However, we have a few tests for DOM bindings, and we should definitely add tests for newly added API, so this patch should include unit tests.
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:51
> + return wrap(obj);</span >
Do not use wrap here, use wrapClientRect directly. This is not a polymorphic object
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:209
> + WebCore::ClientRect* item = WebKit::core(self);
> + gfloat result = item->top();
> + return result;</span >
This is the pattern used by the generated code, but we can do better, this could be just:
return WebKit::core(self)->top();
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:218
> + WebCore::ClientRect* item = WebKit::core(self);
> + gfloat result = item->right();
> + return result;</span >
And same here and all other getters.
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.h:54
> + *</span >
Atogenerated API was not very well documented, but that's not a excuse to no properly document new API :-)
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.h:56
> +**/</span >
Since: 2.18
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.h:65
> +**/</span >
Since: 2.18 everywhere
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRectPrivate.h:21
> +#ifndef WebKitDOMClientRectPrivate_h
> +#define WebKitDOMClientRectPrivate_h</span >
Use #pragma once in private headers
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:128
> + PROP_BOUNDING_CLIENT_RECT,
> + PROP_CLIENT_RECTS,</span >
These are not attributes, but methods:
// CSSOM View Module API
ClientRectList getClientRects();
ClientRect getBoundingClientRect();
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:1259
> + WebCore::Element* item = WebKit::core(self);
> + RefPtr<WebCore::ClientRect> clientRect = item->getBoundingClientRect();</span >
auto clientRect = WebKit::core(self)->getBoundingClientRect();
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:1260
> + return WebKit::kit(clientRect.get());</span >
Amd here you would use ptr() instead of get() because getBoundingClientRect() returns a Ref<>
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:1263
> +GList* webkit_dom_element_get_client_rects(WebKitDOMElement* self)</span >
We can't do this. In other APIs we can use other types to make it more convenient to use or whatever, but here we are implementing the DOM, so we should follow whatever the idl says. getClientRects() returns a ClientRectList, that contains a read-only attribute length and one method item(). So, you should also add WebKitDOMClientRectList, and return that from this method. See WebKitDOMHTMLCollection for an example.
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMPrivate.cpp:169
> +WebKitDOMClientRect* wrap(ClientRect* clientRect)
> +{
> + ASSERT(clientRect);
> +
> + return wrapClientRect(clientRect);
> +}</span >
We don't need this, wrap is never going to be called with a ClientRect because it inherits from WebKitDOMObject, so wrapClientRect is always going to be called by its kit() method.
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMPrivate.h:40
> +class ClientRect;</span >
Ditto.
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMPrivate.h:50
> +WebKitDOMClientRect* wrap(WebCore::ClientRect*);</span >
Ditto.
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/webkitdomdefines.h:338
> +typedef struct _WebKitDOMClientRect WebKitDOMClientRect;
> +typedef struct _WebKitDOMClientRectClass WebKitDOMClientRectClass;</span >
typedefs in this file are alphabetically sorted.</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>