[webkit-reviews] review denied: [Bug 43005] AX: Support methods for web apps to interact with the native accessibility APIs : [Attachment 67736] patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 43005: AX: Support methods for web apps to interact with the native
accessibility APIs
https://bugs.webkit.org/show_bug.cgi?id=43005

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=67736&action=review

Seems reasonable.  Some nits below.

> WebCore/accessibility/AXObjectCache.h:47
> +#if PLATFORM(MAC)
> +#define SUPPORTS_JSACCESSIBILITY 1
> +#else
> +#define SUPPORTS_JSACCESSIBILITY 0
> +#endif

This isn't the way we usually enable features.	We usually use ENABLE macros?

> WebCore/accessibility/Accessibility.cpp:60
> +    Document* doc = m_frame->document();

Please use document as the variable name for the document.

> WebCore/accessibility/Accessibility.cpp:62
> +    if (!doc)
> +	   return;

m_frame->document() can never be null.

> WebCore/accessibility/Accessibility.cpp:64
> +    AccessibilityObject* root =
doc->axObjectCache()->getOrCreate(doc->renderer());

Is there any chance of this code running JavaScript?  If so, we need to hold a
reference to the document.

> WebCore/accessibility/Accessibility.cpp:75
> +    if (!doc)
> +	   return;

m_frame->document() can never be null.

> WebCore/accessibility/Accessibility.idl:33
> +

This line sounds extra.

> WebCore/accessibility/ScreenReader.h:47
> +private:

missing a newline above here.