[webkit-reviews] review denied: [Bug 72754] Send an AXCheckedStateChanged notification when the aria-checked attribute changes. : [Attachment 117061] Patch

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied David Tseng
<dtseng at google.com>'s request for review:
Bug 72754: Send an AXCheckedStateChanged notification when the aria-checked
attribute changes.
https://bugs.webkit.org/show_bug.cgi?id=72754

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

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


this test will likely fail on mac, windows and gtk so you should ask this test
to those Skipped lists

> Source/WebCore/dom/Element.cpp:778
>      } else if (attrName == aria_selectedAttr)

no brackets for the else if since it's a one liner

> LayoutTests/accessibility/aria-checkbox-sends-notification.html:18
> +		   return;

no need to special case out AXLayoutComplete.
Just check that CheckedStateChange has been seen two times.

> LayoutTests/accessibility/aria-checkbox-sends-notification.html:20
> +

notificationCount++ should suffice (instead of += 1)

> LayoutTests/accessibility/aria-checkbox-sends-notification.html:26
> +

no need to add a test complete yourself. the js-test-post should do that

> LayoutTests/accessibility/aria-checkbox-sends-notification.html:41
> +	   }, 10);

i would remove this block. the test will timeout anyway if it doesn't finish,
and this block may mask a true failure
if we're not receiving the notifications something is wrong

> LayoutTests/accessibility/aria-checkbox-sends-notification.html:47
> +	   }, false);

there's no need to do this through an addEventListener. you can just call
runTest() and it should work just fine

> LayoutTests/accessibility/aria-checkbox-sends-notification.html:57
> +

you're missing the js-test-post include here