[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 #302921|review?                     |review-
              Flags|                            |

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

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

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.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:51
> +    return wrap(obj);

Do not use wrap here, use wrapClientRect directly. This is not a polymorphic object

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:209
> +    WebCore::ClientRect* item = WebKit::core(self);
> +    gfloat result = item->top();
> +    return result;

This is the pattern used by the generated code, but we can do better, this could be just:

return WebKit::core(self)->top();

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.cpp:218
> +    WebCore::ClientRect* item = WebKit::core(self);
> +    gfloat result = item->right();
> +    return result;

And same here and all other getters.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.h:54
> + *

Atogenerated API was not very well documented, but that's not a excuse to no properly document new API :-)

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.h:56
> +**/

Since: 2.18

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRect.h:65
> +**/

Since: 2.18 everywhere

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRectPrivate.h:21
> +#ifndef WebKitDOMClientRectPrivate_h
> +#define WebKitDOMClientRectPrivate_h

Use #pragma once in private headers

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:128
> +    PROP_BOUNDING_CLIENT_RECT,
> +    PROP_CLIENT_RECTS,

These are not attributes, but methods:

// CSSOM View Module API                                                                                                                                                                  
ClientRectList getClientRects();
ClientRect getBoundingClientRect();

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:1259
> +    WebCore::Element* item = WebKit::core(self);
> +    RefPtr<WebCore::ClientRect> clientRect = item->getBoundingClientRect();

auto clientRect = WebKit::core(self)->getBoundingClientRect();

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:1260
> +    return WebKit::kit(clientRect.get());

Amd here you would use ptr() instead of get() because getBoundingClientRect() returns a Ref<>

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:1263
> +GList* webkit_dom_element_get_client_rects(WebKitDOMElement* self)

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.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMPrivate.cpp:169
> +WebKitDOMClientRect* wrap(ClientRect* clientRect)
> +{
> +    ASSERT(clientRect);
> +
> +    return wrapClientRect(clientRect);
> +}

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.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMPrivate.h:40
> +class ClientRect;

Ditto.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMPrivate.h:50
> +WebKitDOMClientRect* wrap(WebCore::ClientRect*);

Ditto.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/webkitdomdefines.h:338
> +typedef struct _WebKitDOMClientRect WebKitDOMClientRect;
> +typedef struct _WebKitDOMClientRectClass WebKitDOMClientRectClass;

typedefs in this file are alphabetically sorted.

-- 
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/20170228/e413a4ab/attachment-0001.html>