[webkit-reviews] review denied: [Bug 129250] [ATK] Utilize AtkTableCell to expose directly AccessibilityTableCell to AT. : [Attachment 225244] patch

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has denied Krzysztof Czech
<k.czech at samsung.com>'s request for review:
Bug 129250: [ATK] Utilize AtkTableCell to expose directly
AccessibilityTableCell to AT.
https://bugs.webkit.org/show_bug.cgi?id=129250

Attachment 225244: patch
https://bugs.webkit.org/attachment.cgi?id=225244&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=225244&action=review


Looks mostly good to me. I've got just a few comments before the r+

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceTableCell.cpp:57
> +    if (!axObject || !axObject->isAccessibilityRenderObject())

You should check isAccessibilityTableCell here, instead of
isAccessibilityRenderObject

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceTableCell.cpp:72
> +    if (!axObject || !axObject->isAccessibilityRenderObject())

Ditto.

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:488
> +static void convertGPtrArrayToVector(const GPtrArray* array,
Vector<AccessibilityUIElement>& elements)

This function is only used from the new code added below, so I think we should
put it inside #if ATK_CHECK_VERSION(2,11,90) guards

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:38
> +#include <WebKit2/WKBundleFrame.h>

This include is only used from the new code added below, so I think we should
put it inside #if ATK_CHECK_VERSION(2,11,90) guards

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:554
> +static Vector<RefPtr<AccessibilityUIElement>> convertGPtrArrayToVector(const
GPtrArray* array)

This function is only used from the new code added below, so I think we should
put it inside #if ATK_CHECK_VERSION(2,11,90) guards

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:564
> +static JSValueRef convertToJSObjectArray(const
Vector<RefPtr<AccessibilityUIElement>>& children)

Ditto

> Tools/efl/jhbuild.modules:344
> +    <branch module="pub/GNOME/sources/atk/2.11/atk-2.11.90.tar.xz"
version="2.11.9"

version should be "2.11.90"