Created attachment 216024 [details] patch

Attachment 216024 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp', u'Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp']" exit_code: 1 Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:439: Missing space before ( in for( [whitespace/parens] [5] Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:449: Missing space before ( in for( [whitespace/parens] [5] Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:455: Missing space before ( in for( [whitespace/parens] [5] Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:374: Missing space before ( in for( [whitespace/parens] [5] Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:384: Missing space before ( in for( [whitespace/parens] [5] Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:390: Missing space before ( in for( [whitespace/parens] [5] Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:398: Missing space before ( in for( [whitespace/parens] [5] Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:399: Missing space before ( in for( [whitespace/parens] [5] Total errors found: 8 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 216025 [details] patch

I provided a proposed patch of missing implementation of table-related* functions. Generally it seems not every function could be implemented (example, attributesOfRows, attributesOfColumns, attributesOfHeader). Looks like columns, rows and header are not exposed in ATK so far. So I guess we should provide new baselines for: accessibility/table-attributes.html accessibility/table-sections.html

Comment on attachment 216025 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=216025&action=review (In reply to comment #4) > I provided a proposed patch of missing implementation of table-related* functions. Generally it seems not every function could be implemented (example, attributesOfRows, attributesOfColumns, attributesOfHeader). Looks like columns, rows and header are not exposed in ATK so far. > > So I guess we should provide new baselines for: > accessibility/table-attributes.html > accessibility/table-sections.html I think that's right. Please consider my review below though. Thanks! > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:382 > +static void getRowHeaders(AtkObject* accessible, Vector<AccessibilityUIElement>& rowHeaders) This function only works for AtkTable, so I think you can already expect that type as the first parameter and avoid the casts below. Just make sure you check and cast properly to AtkTable at the caller points Also, I think this is one of those cases where you can use the "move semantics" feature of C++11, which is already supported by Vector through its move contructor Vector(Vector&&). See https://www.webkit.org/blog/3172/webkit-and-cxx11/ > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:384 > + for (int row = 0; row < atk_table_get_n_rows(ATK_TABLE(accessible)); ++row) I think it's better to put the result of get_n_rows() in a variable and use it in the limit condition in the loop, to avoid too many calls to this function. > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:388 > +static void getColumnHeaders(AtkObject* accessible, Vector<AccessibilityUIElement>& columnHeaders) Same considerations than before here (and in the rest of the patch) > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:400 > + for (int row = 0; row < atk_table_get_n_rows(ATK_TABLE(accessibilityObject)); ++row) > + for (int column = 0; column < atk_table_get_n_columns(ATK_TABLE(accessibilityObject)); ++column) > + visibleCells.append(uiElement->cellForColumnAndRow(column, row)); Please use braces for the outermost loop. Also, you can store the get_n_rows() and get_n_columns() in variables too here > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:968 > + Vector<AccessibilityUIElement> columnHeaders; > + getColumnHeaders(m_element, columnHeaders); If the C++11 thing works, this will become one line :) > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:979 > + Vector<AccessibilityUIElement> rowHeaders; > + getRowHeaders(m_element, rowHeaders); Ditto. > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1002 > + Vector<AccessibilityUIElement> visibleCells; > + getVisibleCells(this, visibleCells); Ditto.

Created attachment 216039 [details] patch

> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:382 > > +static void getRowHeaders(AtkObject* accessible, Vector<AccessibilityUIElement>& rowHeaders) > > This function only works for AtkTable, so I think you can already expect that type as the first parameter and avoid the casts below. Just make sure you check and cast properly to AtkTable at the caller points Good point. Done. > > Also, I think this is one of those cases where you can use the "move semantics" feature of C++11, which is already supported by Vector through its move contructor Vector(Vector&&). See https://www.webkit.org/blog/3172/webkit-and-cxx11/ Very good idea. Done. > > > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:384 > > + for (int row = 0; row < atk_table_get_n_rows(ATK_TABLE(accessible)); ++row) > > I think it's better to put the result of get_n_rows() in a variable and use it in the limit condition in the loop, to avoid too many calls to this function. > You are right. Done. > > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:388 > > +static void getColumnHeaders(AtkObject* accessible, Vector<AccessibilityUIElement>& columnHeaders) > > Same considerations than before here (and in the rest of the patch) > > > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:400 > > + for (int row = 0; row < atk_table_get_n_rows(ATK_TABLE(accessibilityObject)); ++row) > > + for (int column = 0; column < atk_table_get_n_columns(ATK_TABLE(accessibilityObject)); ++column) > > + visibleCells.append(uiElement->cellForColumnAndRow(column, row)); > > Please use braces for the outermost loop. Also, you can store the get_n_rows() and get_n_columns() in variables too here Right, Done. > > > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:968 > > + Vector<AccessibilityUIElement> columnHeaders; > > + getColumnHeaders(m_element, columnHeaders); > > If the C++11 thing works, this will become one line :) Done. > > > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:979 > > + Vector<AccessibilityUIElement> rowHeaders; > > + getRowHeaders(m_element, rowHeaders); > > Ditto. Done. > > > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1002 > > + Vector<AccessibilityUIElement> visibleCells; > > + getVisibleCells(this, visibleCells); > > Ditto. Done. Thanks.

Comment on attachment 216039 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=216039&action=review Looks good, except some nitpicks that would be great to fix. Same considerations apply to WKTR > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:374 > + for (Vector<AccessibilityUIElement>::iterator it = elements.begin(); it != elements.end(); ++it) { Sorry I haven't realized of this first, but as you don't need to modify this object, you can use const_iterator here instead > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:387 > + I think this blank line is not needed > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:399 > + Ditto > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:406 > +static Vector<AccessibilityUIElement> getVisibleCells(AccessibilityUIElement* uiElement) In this file, we normally call this parameter "element", not "uiElement" > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:987 > + Extra line not needed > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:997 > + Ditto. > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1019 > + Ditto

Created attachment 216167 [details] patch

Attachment 216167 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp', u'Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp']" exit_code: 1 Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:596: Should have a space between // and comment [whitespace/comments] [4] Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:599: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 216168 [details] patch

Applied all suggestions.

Comment on attachment 216168 [details] patch Lgtm. Thanks

Comment on attachment 216168 [details] patch Clearing flags on attachment: 216168 Committed r158742: <http://trac.webkit.org/changeset/158742>

All reviewed patches have been landed. Closing bug.