[webkit-reviews] review granted: [Bug 51859] WK2: Support Accessibility : [Attachment 77929] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted chris fleizach <cfleizach at apple.com>'s
request for review:
Bug 51859: WK2: Support Accessibility
https://bugs.webkit.org/show_bug.cgi?id=51859

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=77929&action=review

This overall looks really good. Maybe Anders can give it a once over as well.

> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:31
> +#import <AppKit/NSColor.h>

What is this for?  It doesn't seem related.

> WebCore/accessibility/AccessibilityScrollView.cpp:127
> +}
> +
> +    
> +AccessibilityObject* AccessibilityScrollView::accessibilityHitTest(const
IntPoint& point) const

Extra space.

> WebCore/accessibility/AccessibilityScrollbar.cpp:39
> +AccessibilityScrollbar::AccessibilityScrollbar(Scrollbar *scrollbar)

* on the wrong side.

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1027
>  
> +    else if (m_object->isScrollView())
> +	   objectAttributes = scrollViewAttrs;

The newline seems unnecessary.

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1125
> +    }
> +    else {

The } should be on the same line as the else.

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1131
> +	       point = [[view window] convertBaseToScreen: [view convertPoint:
point toView:nil]];

We usually don't put a space after the :.

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1459
> +    if (scroll->platformWidget())
> +	   return scroll->platformWidget();
> +    else
> +	   return [self remoteAccessibilityParentObject];

The else is not necessary here.

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1730
>	   }
> +	   else if (m_object->isScrollView()) {
> +	       AccessibilityObject::AccessibilityChildrenVector children =
m_object->children();

} should be on the same line as the else if.