[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