[webkit-reviews] review granted: [Bug 37040] AX: need to send selected children change notification when aria-selected changed : [Attachment 52556] 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 37040: AX: need to send selected children change notification when
aria-selected changed
https://bugs.webkit.org/show_bug.cgi?id=37040

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    bool nodeIsAriaType(Node*, String role);

The type should be const String&. If the term for the argument really role,
then I'd think you'd want it to be called nodeHasRole rather than
nodeIsAriaType. Or nodeHasARIAType.

> -	   if (renderer->isTextControl())
> -	       return
renderer->document()->axObjectCache()->getOrCreate(renderer);
> +	   if (renderer->isTextControl() 
> +	       || (renderer->isListBox() ||
axObjectCache()->nodeIsAriaType(renderer->node(), "listbox")))
> +	       return axObjectCache()->getOrCreate(renderer);

This seems like a pretty strange set of conditions. I think this rule should be
implemented with a helper function, just to give you a place to put a comment.
And the comment can explain why these three things are special.

> +    AccessibilityChildrenVector childObjects = children();
> +    for (unsigned k = 0, childrenSize = childObjects.size(); k <
childrenSize; ++k) {

Normally we'd just initialize childrenSize outside the loop. It's neat to have
it scoped like this, but clearer written the more-conventional way.