[webkit-reviews] review granted: [Bug 121413] [ATK] Extends atk value interface to return proper checkbox states. : [Attachment 211747] Patch

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has granted Krzysztof Czech
<k.czech at samsung.com>'s request for review:
Bug 121413: [ATK] Extends atk value interface to return proper checkbox states.
https://bugs.webkit.org/show_bug.cgi?id=121413

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

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=211747&action=review


The patch looks good already, yet there are some minor changes that I think we
should do before landing

> LayoutTests/ChangeLog:8
> +	   Sharing mac tests with other ports (GTK/EFL).

Great!

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceValue.cpp:39
> +static float webkitAccessibleValueGetCurrentValue(AccessibilityObject*
coreObject)

I would probably call this webkitAccessibleValueValueForAccessibilityObject(),
to make it clearer from the caller (and to make it more explicit in the
ChangeLog that you added a new function here)

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:1038
> +    if (role == SliderRole || role == SpinButtonRole || role ==
ScrollBarRole || role == ProgressIndicatorRole || role == CheckBoxRole)

I think we should replace this condition with (coreObject->supportsRangeValue()
|| coreObject->isCheckboxOrRadio())