Implements, get_column_span, get_row_span, get_position, get_table.
<rdar://problem/16196432>
Created attachment 225646 [details] patch
Created attachment 225647 [details] patch
Created attachment 225648 [details] patch
Created attachment 225649 [details] patch
Comment on attachment 225649 [details] patch 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.
Comment on attachment 225649 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=225649&action=review > Source/WebCore/ChangeLog:8 > + Implemented missing API of AtkTableCell. too many spaces after Implemented > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceTableCell.cpp:134 > + AtkObject* table = atk_object_get_parent(axObject->wrapper()); should you verify that this is actually the table element before returning? > Source/WebCore/accessibility/atk/WebKitAccessibleUtil.h:63 > + AccessibilityObject* axObject = nullptr; \ It's a little non-intuitive that a local variable will be declared inside the macro, that you reference outside the macro. Maybe it would be better to write a function that does this same check AccessibilityObject* axObject = verifyAccessibilityObjectIsValid(...); if (!axObject) return;
Created attachment 225773 [details] patch
(In reply to comment #6) > (From update of attachment 225649 [details]) > 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 Done. > > > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceTableCell.cpp:103 > > +gboolean webkitAccessibleTableCellGetPosition(AtkTableCell* cell, gint *row, gint* column) > > misplaced * Done > > > 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. Done
(In reply to comment #7) > (From update of attachment 225649 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225649&action=review > > > Source/WebCore/ChangeLog:8 > > + Implemented missing API of AtkTableCell. Done. > > too many spaces after Implemented > > > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceTableCell.cpp:134 > > + AtkObject* table = atk_object_get_parent(axObject->wrapper()); > > should you verify that this is actually the table element before returning? Yes exactly, you are right. Thanks. > > > Source/WebCore/accessibility/atk/WebKitAccessibleUtil.h:63 > > + AccessibilityObject* axObject = nullptr; \ > > It's a little non-intuitive that a local variable will be declared inside the macro, that you reference outside the macro. > > Maybe it would be better to write a function that does this same check Yes, this sounds as good idea. We already have similar function that returns AccessibilityObject for respective AtkObject and it's called webkitAccessibleGetAccessibilityObject. I think it can be slightly extended for adding some additional checks. I think it should be modified in a separate patch because it touches many files that are not related to this changes. > > AccessibilityObject* axObject = verifyAccessibilityObjectIsValid(...); > if (!axObject) > return; Thanks
Comment on attachment 225773 [details] patch Looks good to me now. Thanks!
Comment on attachment 225773 [details] patch Clearing flags on attachment: 225773 Committed r165107: <http://trac.webkit.org/changeset/165107>
All reviewed patches have been landed. Closing bug.