[webkit-reviews] review denied: [Bug 79080] Row and column headers not exposed by WebCore accessibility : [Attachment 131192] Remove alteration of prepare-ChangeLog script

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied Aaron Leventhal
<aaronlevbugs at gmail.com>'s request for review:
Bug 79080: Row and column headers not exposed by WebCore accessibility
https://bugs.webkit.org/show_bug.cgi?id=79080

Attachment 131192: Remove alteration of prepare-ChangeLog script
https://bugs.webkit.org/attachment.cgi?id=131192&action=review

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


> LayoutTests/ChangeLog:9
> +	   * accessibility/table-headers.html: Added.

These tests, test too many things and are unclear in their results. 
Dumping the whole AX tree makes it impossible to figure out what's happening in
the test.
You should make specific calls to verify that specific information is correct.
Also, i think there should be a separate test per <table> because they test
different things. It will make it much easier to debug future problems

What platforms does this test actually work on?

> Source/WebCore/ChangeLog:5
> +

they are exposed just fine on the Mac, through AXColumnHeaderUIElements and
AXRowHeaderUIElements. This change log needs more information as to what
platform its affecting and what is actually missing.

> Source/WebCore/accessibility/AccessibilityObject.h:351
> +    // accessible table interface.

this comment could be on one line

> Source/WebCore/accessibility/AccessibilityObject.h:355
>      virtual bool isDataTable() const { return false; }

as far as i can tell, isDataTable() does not need to be on AccessibilityObject.
I see it only used within AccessibilityTable. we can move this method to
AccessibilityTable. 
The double negative comment is hard to understand. 
The comment should read "Returns true if heuristics determine the HTML table
appears to be exposing a grid of data, and not used for layout purposes."

> Source/WebCore/accessibility/AccessibilityObject.h:358
> +    // Return true if something is a cell, a rowheader or a columnheader

comments need periods

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1737
> +	   return false;

checking m_renderer isn't important here. there's nothing in this method that
uses m_renderer. ariaRoleAttribute() takes care of that

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1739
> +    AccessibilityRole ariaRole = ariaRoleAttribute();

this can all be made into a one line function

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3289
> +{

i don't think you need this method.
bool AccessibilityObject::isLandmark() const
already exists

> Source/WebCore/accessibility/AccessibilityRenderObject.h:275
> +    

both of these do not need to be on the render object. they should be on
AccessibilityObject

> Source/WebCore/accessibility/AccessibilityTable.cpp:86
>	   return false;

this line makes it seem like to me that something can still be a table even if
it's role is a landmark role. that seems wrong

> Source/WebCore/accessibility/AccessibilityTable.h:91
> +    // Return true if it is viable to expose an a11y table interface for

a11y is not a word. please use a real word.

> Source/WebCore/accessibility/AccessibilityTable.h:94
>      bool isTableExposableThroughAccessibility() const;

this comment is wrong. this is a private method to determine if the table
should be exposed as a table interface. isAccessibilityTable() returns the
result of this method (which is stored in a bool)

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:-98
> -    return CellRole;

There is a lot of work that is being performed below. the result should be
cached whether it's a Cell, ColumnHeader or RowHeader, or if it should ask
AccessibilityRenderObject::roleValue() for the answer so that this does not
need to be calculated every time

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:101
> +    static_cast<HTMLTableCellElement*>(node());

this line doesn't need to be on two lines

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:103
> +	   ASSERT_NOT_REACHED();

this seems like a useless check. when would cellElement ever == 0? i suppose
only if node() was nil, so it seems like you should just check if node() == 0
and return

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:134
> +    // based on the position of the <th> in the table

comments should end with periods.

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:152
> +	    static_cast<AccessibilityTable*>(parentTable());

you should use toAccessibilityTable() instead of this and this can be on one
line

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:164
> +    Element* nextCellElt = static_cast<Element*>(nextCellInRow->node());    


what's an "Elt"

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:165
> +    if (!nextCellElt) {

why are you casting to Element? hasTagName exists on node()

> Source/WebCore/accessibility/AccessibilityTableCell.h:44
> +    // Return true if either table cell or column/row header

this should be a full sentence

> Tools/Scripts/prepare-ChangeLog:65
> +use lib "Tools/Scripts/";

I doubt this fix requires a change to prepare-Changelog. if it does, it should
be in a separate patch