[webkit-reviews] review requested: [Bug 72589] [GTK] Expose accessibility hierarchy in WebKit2 to ATK/AT-SPI based ATs : [Attachment 121681] Patch proposal

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 72589: [GTK] Expose accessibility hierarchy in WebKit2 to ATK/AT-SPI based
ATs
https://bugs.webkit.org/show_bug.cgi?id=72589

Attachment 121681: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=121681&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
Thanks again Carlos for your feedback. Now attaching another patch addressing
the issues you pointed out, as well as incorporating Martin's suggestion of
adding at-spi2-{core|atk} (and the needed version) to jhbuild modules.

As for the g_spawn_sync thing (pointed out in a separate comments), I made sure
to close the file descriptor (which was leaking as you noticed) and, finally,
changed the code to use your suggestion of the "killer thread" + the
GIOChannel... but with g_spawn_async(), as you also pointed out in your last
comment (Thanks!)

So, I think we've reached some consensus here, or at least I hope so... :-)

Now, see some comments inline below...

(In reply to comment #11)
> (From update of attachment 120955 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=120955&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:341
> > +static AtkObject* webkitWebViewBaseGetAccessible(GtkWidget *widget)
> 
> GtkWidget *widget -> GtkWidget* widget
>
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:64
> > +static AtkStateSet* webkitWebViewBaseAccessibleRefStateSet(AtkObject
*atkObject)
> 
> AtkObject *atkObject -> AtkObject* atkObject 
>
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:68
> > +	 AtkStateSet* state_set;
> 
> state_set -> stateSet
>
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:83
> > +static gint webkitWebViewBaseAccessibleGetIndexInParent(AtkObject
*atkObject)
> 
> AtkObject *atkObject -> AtkObject* atkObject
> 
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:85
> > +	 g_return_val_if_fail(ATK_IS_OBJECT(atkObject), -1);
> 
> Don't use g_return macros in private static code, note that
atk_object_get_index_in_parent() already has that check, so it doesn't get here
if pointer is not a valid AtkObject.
> 
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:128
> > +	 AtkObject* object =
static_cast<AtkObject*>(g_object_new(WEBKIT_TYPE_WEB_VIEW_BASE_ACCESSIBLE, 0));

> 
> Use ATK_OBJECT() instead of static_cast. Use NULL instead of 0 in
g_object_new() since it's a sentinel
> 
Fixed (Done the same fix in WebPageAccessibilityObject, btw).

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:31
> > +#include <webkit2/WebKitDefines.h>
> 
> Do you need this?
> 
Not actually. Removed.

> > Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:55
> > +	 WebKitWebLoaderClient* loaderClient =
webkit_web_view_get_loader_client(webView);
> 
> Loader client has been already removed, use WebKitWebView::load-changed
instead.
> 
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:58
> > +	 int child_stdout = 0;
> 
> child_stdout -> childStdout
> 
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:113
> > +	 GRefPtr<AtspiAccessible> desktop = atspi_get_desktop(0);
> 
> atspi_get_desktop returns a new ref, so you should use adoptGRef() here.
> 
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:136
> > +	 GRefPtr<AtspiAccessible> currentChild =
atspi_accessible_get_child_at_index(currentParent.get(), 0, 0);
> 
> I think you should use adoptGRef here too.
>
You are right. Fixed.

> Use Test::assertObjectIsDeletedWhenTestFinishes() to make sure you don't leak
anything.
> 
I can't use that, since the test doesn't own the object at all, which has a
ref_count greater than 1 both when retrieving it here and when finishing the
test, since libatspi keeps the full a11y hierarchy cached and so several
references to these objects, which therefore won't have a ref_count of 0 by any
means with finishing the test, so they won't get deleted at that point.

Still, your point has been helpful anyway, since it's true I was leaking one
reference by no using adoptGRef(). Thanks.

> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:78
> > +	 g_return_val_if_fail(ATK_IS_OBJECT(atkObject), -1);
> 
> Don't use g_return macro here.
> 
Fixed.

> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:110

> > +	 accessible->m_page = 0;
> 
> accessible is already initialized to 0 when allocated.
> 
Fixed.

> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:128

> > +	 AtkObject* object =
static_cast<AtkObject*>(g_object_new(WEB_TYPE_PAGE_ACCESSIBILITY_OBJECT, 0));
> 
> Use NULL instead of 0 in g_object_new
> 
Fixed.

> > Tools/Scripts/run-gtk-tests:98
> > +	 paths_to_check = [ "/usr/libexec",
> > +			    "/usr/lib/at-spi2-core",
> > +			    "/usr/lib32/at-spi2-core"
> > +			    "/usr/lib64/at-spi2-core" ]
> 
> Maybe we can use pkg-config --variable=exec_prefix atspi-2 ?

Fixed.