[webkit-reviews] review denied: [Bug 118969] [ATK] Implement tables-related attributesOf*() functions for AccessibilityUIElement : [Attachment 216025] 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 118969: [ATK] Implement tables-related attributesOf*() functions for
AccessibilityUIElement
https://bugs.webkit.org/show_bug.cgi?id=118969

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

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
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.