[webkit-reviews] review denied: [Bug 25989] [Gtk] Enable accessibility in Gtk DRT : [Attachment 31041] Add Accessibility support to GTK DRT

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has denied Jan Alonzo <jmalonzo at gmail.com>'s
request for review:
Bug 25989: [Gtk] Enable accessibility in Gtk DRT
https://bugs.webkit.org/show_bug.cgi?id=25989

Attachment 31041: Add Accessibility support to GTK DRT
https://bugs.webkit.org/attachment.cgi?id=31041&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
> +AtkObject* webkit_accessible_get_focused_element(WebKitAccessible*
accessible)
> +{
> +    if (!accessible->m_object)
> +	   return 0;
> +
> +    // FIXME: update the backing store once we have one.
> +
> +    RefPtr<AccessibilityObject> focusedObj =
accessible->m_object->focusedUIElement();
> +    if (!focusedObj)
> +	   return 0;
> +
> +    return focusedObj->wrapper();
> +}
> +

I can never decide if we should add new API in one patch and its usage in
another, or everything in one patch.

> diff --git a/WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp
b/WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp
> index c78323e..48494c3 100644
> --- a/WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp
> +++ b/WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp
> @@ -28,6 +28,8 @@
>  
>  #include <JavaScriptCore/JSRetainPtr.h>
>  
> +#include <limits.h>
> +
>  // Static Functions
>  

I would put this in a different patch.

> +AccessibilityUIElement AccessibilityUIElement::getChildAtIndex(unsigned
index)
> +{
> +    Vector<AccessibilityUIElement> children;
> +    getChildrenWithRange(children, index, 1);
> +
> +    if (children.size() == 1)
> +	   return children.at(0);
> +
> +    return 0;
> +}

Mmm, why can't you just use atk_object_ref_accessible_child with index here?

> +AccessibilityUIElement AccessibilityUIElement::parentElement()
> +{
> +    g_assert(m_element);

I guess we should use either ASSERT or g_assert, and stick to that.

> +
> +double AccessibilityUIElement::intValue()
> +{
> +    GValue value = { 0, { { 0 } } };

I suppose { 0, } like in C does not work in C++.

> +
> +    if (!ATK_IS_VALUE(m_element))
> +	   return 0.0f;
> +
> +    atk_value_get_current_value(ATK_VALUE(m_element), &value);
> +    return g_value_get_double(&value);

How do we know the GValue has a double value? Shouldn't we do a
G_VALUE_HOLDS_DOUBLE before?

> @@ -432,7 +434,10 @@ static void webViewWindowObjectCleared(WebKitWebView*
view, WebKitWebFrame* fram
>      assert(gLayoutTestController);
>  
>      gLayoutTestController->makeWindowObject(context, windowObject,
&exception);
> -    assert(!exception);
> +    ASSERT(!exception);
> +
> +    accessibilityController->makeWindowObject(context, windowObject,
&exception);
> +    ASSERT(!exception);

Stupid question, where is this created? :D

The patch looks pretty good to me, but I'll mark it as r- while we work out the
last details.