[webkit-reviews] review denied: [Bug 79080] Row and column headers not exposed by WebCore accessibility : [Attachment 133916] 1) Address review comments, 2) Do work to determine cell role only once, 3) Separate/clarify tests and provided -expected files for Chromium-win port

[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 133916: 1) Address review comments, 2) Do work to determine cell
role only once, 3) Separate/clarify tests and provided -expected files for
Chromium-win port
https://bugs.webkit.org/attachment.cgi?id=133916&action=review

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


there's no change logs in this patch.

There's a method in js-pre that allows you to output the purpose of the test.
you'll see that in the platform/mac/accessibility tests. I would add those to
each of the tests, so it's clear what the test is for (and where to find that
purpose)

> b/LayoutTests/accessibility/table-headers-aria-div-grid.html:15
> +	   <span role="gRidcell">Cell</span>

strange capitalization here

> b/LayoutTests/accessibility/table-headers-aria-div-grid.html:35
> +	   var table  = accessibilityController.focusedElement.childAtIndex(0);


too much whitespace after table

> LayoutTests/accessibility/table-headers-aria-table-grid.html:21
> +	   <th role="coluMnheader">Columnheader</th>

strange capitalization here

> LayoutTests/accessibility/table-headers-aria-table-grid.html:26
> +	   <th role="coluMnheader">Columnheader</th>

strange capitalization here

> LayoutTests/accessibility/table-headers-aria-table-grid.html:35
> +	   var table  = accessibilityController.focusedElement.childAtIndex(0);


too much white space

> LayoutTests/accessibility/table-headers-heuristic-column-only.html:29
> +	   var table  = accessibilityController.focusedElement.childAtIndex(0);


spacing

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:118
> +    // 1. If grandparent is a <thead> this is a column header, for both <th>
and <td>

I would remove the --- from the comment as well as the empty line. There's
nothing extraordinary about this comment

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:122
> +    ASSERT(tr->hasTagName(trTag)); // DOM has element even if not in doc.

place comment before the assert line. also that comment does not explain why
you need to assert

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:129
> +    // 2. All <td> elements not inside a <thead> are cells 

comments end in period

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:130
> +    if (currentNode->hasTagName(tdTag))

this is the 2nd time you called hasTagName(tdTag). you should probably cache it


> Source/WebCore/accessibility/AccessibilityTableCell.cpp:135
> +    // 3. Check scope attribute

ditto for this comment block. comments end in period

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:153
> +	   return ColumnHeaderRole; // First row, but not first col.

i would put comment above if block

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:155
> +	   return RowHeaderRole; // First col, but not first row.

i would put comment before if block

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:164
> +    AccessibilityTable* parentTableAcc =
toAccessibilityTable(parentTable());

the name parentTableAcc is strange to me because it abbreviates Accessibility
in a non-standard way in webkit. i would just call it parentTable or
parentTableObject

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:165
> +    ASSERT(parentTableAcc); // Cannot be null if isTableCell().

comment before Assert

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:171
> +	   parentTableAcc->cellForColumnAndRow(nextColInRow, rowRange.first);

does not seem like this needs to be on two lines