[webkit-reviews] review granted: [Bug 31299] WAI-ARIA: implement treegrid : [Attachment 44760] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 31299: WAI-ARIA: implement treegrid
https://bugs.webkit.org/show_bug.cgi?id=31299

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    for (int k = index+1; k < rowCount; ++k) {

We put spaces around operators like the "+" here.

> +	   if (row->hierarchicalLevel() == (level+1))
> +	       disclosedRows.append(row);
> +	   // Stop at the first row that doesn't match the correct level.
> +	   else
> +	       break;

I would reverse the logic and says if != break, then put the append outside the
if, avoiding the else. Also, I would omit the parentheses around "level+1" and
include the spaces around the "+".

> +    // If the level is 1 or less, than nothing discloses this row.
> +    unsigned level = hierarchicalLevel();
> +    if (level < 2)
> +	   return 0;

Comment says 1 or less, code should say <= 1 to match it.

> +    for (int k = index-1; k >= 0; --k) {

Should have a space around the "-".

> +	   if (row->hierarchicalLevel() == (level-1))

Space around the "-" and no parentheses around "level - 1".

> +    const AtomicString& ariaMultiSelectable =
getAttribute(aria_multiselectableAttr).string();

Please remove the ".string()" here. It causes the compiler to create a new
temporary AtomicString by converting to String and back to AtomicString, but
without it everything should work fine.

>      // Setting selected rows only works on trees for now.
> -    if (roleValue() != TreeRole)
> +    AccessibilityRole role = roleValue();
> +    if (role != TreeRole && role != TreeGridRole && role != TableRole)
>	   return;

Comment no longer makes sense, since it works for tables now.
>      
> -    bool isMultiselectable =
elementAttributeValue(aria_multiselectableAttr);
> +    bool isMultiselectable = isMultiSelectable();

This works only because you treated "multiselectable" as one word in the local
variable and two words in the function name. The two should be capitalized
consistently (all lowercase in both cases) and then you either need to
eliminate the local variable or use "this->" syntax to call the function.

The override of the supportsSelectedRows function in AccessibilityARIAGrid.h
should be private. It's programming mistake to ever call it on an
AccessibilityARIAGrid* and it would be good to have the compiler catch such a
mistake.

All of these are minor quibbles. r=me as is