[webkit-reviews] review denied: [Bug 35422] [Gtk] Layout tables should indicate that they are not data tables via an object attribute : [Attachment 71628] Patch proposal + Unit 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 35422: [Gtk] Layout tables should indicate that they are not data tables
via an object attribute
https://bugs.webkit.org/show_bug.cgi?id=35422

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

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

i agree with the refactoring. just have some questions inline

> WebCore/accessibility/AccessibilityTable.cpp:87
> +    return m_isAccessibilityTable;

where does m_isAccessibilityTable get set now that
isTableExposableThroughAccessibility() is const

> WebCore/accessibility/AccessibilityTable.cpp:95
> +    // don not consider it a data table is it has an ARIA role

spelling. end sentence with period. capitalize start of sentence

> WebCore/accessibility/AccessibilityTable.cpp:100
> +    // Only "data" tables should be exposed as tables.

sentence should start with capital letter

> WebCore/accessibility/AccessibilityTable.cpp:258
> +

fix this comment (even though it's not yours to look more like a real sentence

> WebCore/accessibility/AccessibilityTable.cpp:263
> +    // expose it as a table, unless, of course, the aria role is a table

ditto

> WebCore/accessibility/AccessibilityTable.cpp:266
> +

does this still account for role="table"?

> WebCore/accessibility/AccessibilityTable.cpp:272
> +

you can probably do this in one line

return tableNode && tableNode->hasTagName(tableTag)

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:336
> +    if (coreObject) {

you should use an early return here if !coreObject)

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:344
> +	   // Technologies know when an exposed table is not data table

end the sentence with a period

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:345
> +	   if (coreObject->isAccessibilityTable() &&
!coreObject->isDataTable()) {

not sure why you want an AXTable, but not a dataTable... wouldn't that mean the
layout-guess is false, because you know it's an accessibility table.
can you explain to me why this is right?

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:347
> +	       attributeSet = addAttributeToSet(attributeSet, "layout-guess",
value.utf8().data());

can you not do 


attributeSet = addAttributeToSet(attributeSet, "layout-guess", "true");
?