[webkit-reviews] review denied: [Bug 57463] [GTK] ARIA tables not exposing cells as HTML tables do : [Attachment 87839] Patch proposal + Layout test

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 57463: [GTK] ARIA tables not exposing cells as HTML tables do
https://bugs.webkit.org/show_bug.cgi?id=57463

Attachment 87839: Patch proposal + Layout test
https://bugs.webkit.org/attachment.cgi?id=87839&action=review

------- Additional Comments from chris fleizach <cfleizach at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87839&action=review

> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:113
> +

!child check is not needed

> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:115
> +	       if (appendedRows.contains(row))

this check could be in the previous check. there's no need to cast to a
AccessibilityTableRow at this point

> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:119
> +	       unsigned rowCellCount = row->children().size();

Store instead of "store"

> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:123
> +	       row->setRowIndex((int)columnCount);

static_cast
also i don't think the rowIndex is the same as the columnCount. The row index
should be m_rows.size()

> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:125
> +	       m_children.append(row->children());

why is all this logic duplicated from addChild. Is there any difference with
what you added?

> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:450
> +    case GridRole:

i believe you removed this in another patch