Hello. It seems there are not methods named webkit_dom_element_get_bounding_client_rect() in webkitdom to be used by a web extension. I currently use webkit_dom_element_get_offset_{left,top} recursively, but it does not take CSS3 transformations into account. Could you please add the function webkit_dom_element_get_bounding_client_rect() so that we can get the exact position of an element from a web extension? The method is defined [here in webkit](https://github.com/WebKit/webkit/blob/1719216d75e6c08a8a6358062a5418d304e71e97/Source/WebCore/dom/Element.h#L177). Thanks.
This would be a good newcomers task if anyone is looking for one, you'd just need to modify WebKitDOMElement.[cpp,h] in Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/ to add a wrapper for the function mentioned in the first comment, then update webkitdomgtk-4.0-sections.txt in Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/docs.
I'd be interested in working on this, as I've had to resort to the same buggy workaround.
Go ahead! This seems like a good newcomers bug. (In reply to comment #1) > This would be a good newcomers task if anyone is looking for one, you'd just > need to modify WebKitDOMElement.[cpp,h] in > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/ to add a wrapper for > the function mentioned in the first comment, then update > webkitdomgtk-4.0-sections.txt in > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/docs. Note the function you'd be wrapping is Element::getBoundingClientRect from Source/WebCore/dom/Element.cpp. Also note that we don't require API tests for exposing new DOM API, like we do for all other new API, so your own manual testing is sufficient for this.
(In reply to comment #2) > I'd be interested in working on this, as I've had to resort to the same > buggy workaround. Thanks. If you could also add the related [`webkit_dom_element_get_client_rects()` function](https://github.com/WebKit/webkit/blob/1719216d75e6c08a8a6358062a5418d304e71e97/Source/WebCore/dom/Element.h#L176) it would be nice. I don't know how `List` are returned to the web extension, but you might ask for help if you don't know too.
(In reply to comment #3) > Also note that we don't require API tests for exposing new DOM API, like we > do for all other new API, so your own manual testing is sufficient for this. Great; how should I wrap the ClientRect class? As a separate WebKitDOMElementClientRect class with getters for each of the six member variables? (In reply to comment #4) > If you could also add the related [`webkit_dom_element_get_client_rects()` > function](https://github.com/WebKit/webkit/blob/ > 1719216d75e6c08a8a6358062a5418d304e71e97/Source/WebCore/dom/Element.h#L176) > it would be nice. Sure.
I just went ahead and made it a WebKitDOMClientRect (not WebKitDOMElementClientRect); that seems to be consistent with the other WebKitDOM* wrappers.
Created attachment 302841 [details] Patch
Comment on attachment 302841 [details] 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....
> > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElement.cpp:1270 > > + size_t clientRectsLength = clientRects.get()->length(); > > GList* clientRectList I guess I clicked the wrong line. I was trying to complain about one line where you declared this: GList *clientRectList instead of this: GList* clientRectList
(In reply to comment #8) > > 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. Not sure what you mean by this. I've fixed the other complaints. > > 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. When in Rome... I've had the same thought while browsing the docs :) > > 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)? Actually, I think the way I implemented it currently is transfer full for both functions... I'm still getting used to WebKit's refcounting system. I'll try to change that to transfer none and transfer container, since that seems more user-friendly.
(In reply to comment #10) > (In reply to comment #8) > > > 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. > > Not sure what you mean by this. I've fixed the other complaints. Try implementing the constructed virtual method, rather than the constructor virtual method: https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#GObjectClass constructed is simpler, but is called after properties have already been set. I don't think it matters in your case.
Created attachment 302921 [details] Patch
That should fix everything you mentioned. I've made both functions transfer full, since WebCore::Element returns newly created ClientRect instances. I'm not sure about the ref-counting stuff; do I need to use WTF::move() in webkit_dom_client_rect_constructed(), or unref anything in the finalizer? I also removed some uses of WTF::getPtr() from the DOMElement getters, since they didn't seem to be necessary.
Comment on attachment 302921 [details] 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.
(In reply to comment #14) > Comment on attachment 302921 [details] > 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. Are there any DOM bindings that do have tests? To use as a guideline. > > > 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(); So I should remove the added properties in WebKitDOMElement.cpp, and leave just the two webkit_dom_element_get_whatever()? > > 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. Okay, makes sense.
Created attachment 302936 [details] Patch
That should be everything except tests (not sure how to do those) and nicer documentation. Adding descriptions to webkit_dom_element_get_foo() is fairly simple, but how detailed should e.g. each property of WebKitDOMClientRect be?
My opinion is that DOM API doesn't need tests, since none of the rest of the API has tests. But Carlos's opinion wins. Look in Tools/TestWebKitAPI/Tests/WebKitGTK+ for e.g. the web extension test as an example.
(In reply to comment #18) > My opinion is that DOM API doesn't need tests, since none of the rest of the > API has tests. But Carlos's opinion wins. It's not a matter of opinion, it's that this is not true. We have DOM API tests, I added the concept of "web process tests" precisely to test DOM API. We currently have: TestDOMNode.cpp TestDOMNodeFilter.cpp TestDOMXPathNSResolver.cpp I agree that we don't need very complex tests, because the actual functionality is in WebCore and already tested by layout tests using the JS API. > Look in > Tools/TestWebKitAPI/Tests/WebKitGTK+ for e.g. the web extension test as an > example. Look at the examples I mentioned above. You need to add two files TestDOMFoo.cpp and DOMFooTest.cpp. The former is the test file that runs in the UI process, it adds the tests cases that normally just load some html and then tun the web process test simply calling WebViewTest::runWebProcessTest(). The latter is the test itself that runs in the web process. You have to add a class derived from WebProcessTest with a method for very tests case and override runTest that dispatches the test cases. To register the tests we use a lib constructor that uses the macro REGISTER_TEST.
(In reply to comment #19) > Look at the examples I mentioned above. You need to add two files > TestDOMFoo.cpp and DOMFooTest.cpp. The former is the test file that runs in > the UI process, it adds the tests cases that normally just load some html > and then tun the web process test simply calling > WebViewTest::runWebProcessTest(). The latter is the test itself that runs in > the web process. You have to add a class derived from WebProcessTest with a > method for very tests case and override runTest that dispatches the test > cases. To register the tests we use a lib constructor that uses the macro > REGISTER_TEST. Thanks for the guidelines; seems pretty straightforward. Is there any way to run only the WebKitDOM tests while I develop? Separately, is it alright to keep tests for both ClientRect and ClientRectList in the same pair of files? Or should I separate them?
(In reply to comment #20) > (In reply to comment #19) > > Look at the examples I mentioned above. You need to add two files > > TestDOMFoo.cpp and DOMFooTest.cpp. The former is the test file that runs in > > the UI process, it adds the tests cases that normally just load some html > > and then tun the web process test simply calling > > WebViewTest::runWebProcessTest(). The latter is the test itself that runs in > > the web process. You have to add a class derived from WebProcessTest with a > > method for very tests case and override runTest that dispatches the test > > cases. To register the tests we use a lib constructor that uses the macro > > REGISTER_TEST. > > Thanks for the guidelines; seems pretty straightforward. Is there any way to > run only the WebKitDOM tests while I develop? Yes, run-gtk-tests allows to run individual tests, you just need to pass the path to the test, for example: Tools/Scripts/run-gtk-tests --verbose WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestDOMNode > Separately, is it alright to keep tests for both ClientRect and > ClientRectList in the same pair of files? Or should I separate them? It's ok to use the same file with two different test cases for example.
Created attachment 303052 [details] Patch
Comment on attachment 303052 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303052&action=review > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDOMClientRect.cpp:56 > +} > + > + > + > +void beforeAll() Too many blank lines here. Just one, please.
Created attachment 303456 [details] Patch
Comment on attachment 303456 [details] Patch Attachment 303456 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3247078 New failing tests: fast/css/target-fragment-match.html
Created attachment 303457 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 303456 [details] 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.
Created attachment 303650 [details] Patch
(In reply to comment #27) > Why is this out of WebKitDOMClientRectTest class? I put it there because it was a helper function rather than a top-level test; I've moved it back. It returned bool because the test functions did as well.
Comment on attachment 303650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303650&action=review > Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMClientRectTest.cpp:26 > +static bool testClientRectPosition(WebKitDOMClientRect* clientRect) This is only used inside the WebKitDOMClientRectTest, it can be a static function in the class instead of here. If you don't want it to look like a test case just renamed to something like checkClientRectPosition. > 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; Test cases are boolean because they might return false to indicate that the test failed, even though most of the times they just r4eturn true and use asserts to make something fail.. But this is just a helper function.
Created attachment 304916 [details] Patch
Comment on attachment 304916 [details] Patch Clearing flags on attachment: 304916 Committed r214215: <http://trac.webkit.org/changeset/214215>
All reviewed patches have been landed. Closing bug.