[Webkit-unassigned] [Bug 72589] [GTK] Expose accessibility hierarchy in WebKit2 to ATK/AT-SPI based ATs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72589


Mario Sanchez Prada <msanchez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #120955|0                           |1
        is obsolete|                            |
 Attachment #120955|review?                     |
               Flag|                            |
 Attachment #121681|                            |review?
               Flag|                            |




--- Comment #24 from Mario Sanchez Prada <msanchez at igalia.com>  2012-01-09 10:34:25 PST ---
Created an attachment (id=121681)
 --> (https://bugs.webkit.org/attachment.cgi?id=121681&action=review)
Patch proposal

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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.