[webkit-reviews] review denied: [Bug 129492] [ATK] Expose missing functionalities of AtkTableCell to AT. : [Attachment 225649] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Mar 3 08:57:19 PST 2014


Mario Sanchez Prada <mario at webkit.org> has denied Krzysztof Czech
<k.czech at samsung.com>'s request for review:
Bug 129492: [ATK] Expose missing functionalities of AtkTableCell to AT.
https://bugs.webkit.org/show_bug.cgi?id=129492

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

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


Thanks for doing this. The patch looks good to me in general, but still needs
some changes IMHO (see below). Also, table cell is a pretty new thing to ATK so
you probably want to use ATK_CHECK_VERSION in some places too.

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceTableCell.cpp:49
> +    accessibilityObjectGetOrReturn(cell, axObject, nullptr);
>  
> -    AccessibilityObject* axObject = core(cell);
> -    if (!axObject || !axObject->isTableCell())
> +    if (!axObject->isTableCell())

I'd rather keep the old idiom in place. This new macro is confusing IMHO,
specially because of containing an out parameter, and thus decreases
readability

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceTableCell.cpp:103
> +gboolean webkitAccessibleTableCellGetPosition(AtkTableCell* cell, gint *row,
gint* column)

misplaced *

> Source/WebCore/accessibility/atk/WebKitAccessibleUtil.h:70
> +    } while (0)

Because of the particular nature of this macro, you can not place the whole
block inside G_STMT_START/END guards which, besides being probably not good
enough for places where the compiler expects the macro to expand in only one
line, looks a bit awkward, IMO

This should go away if you go back to the old idiom, though

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:213
> +    gint row = -1, column= -1, rowSpan = -1, columnSpan = -1;

Please declare each variable in a separate line and use int instead of gint.


More information about the webkit-reviews mailing list