[webkit-reviews] review canceled: [Bug 72589] [GTK] Expose accessibility hierarchy in WebKit2 to ATK/AT-SPI based ATs : [Attachment 120955] Patch proposal
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 9 10:34:24 PST 2012
Mario Sanchez Prada <msanchez at igalia.com> has canceled Mario Sanchez Prada
<msanchez at igalia.com>'s request 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 120955: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=120955&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.
More information about the webkit-reviews
mailing list