Bug 130575 – [ATK] Utilize new AtkValue interface coming in ATK 2.11.92

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Krzysztof Czech]

ATK 2.11.92 introduces some new API and deprecates an old one. It adds AtkRange data type which I guess it could be used to squeeze some properties of a range controls into one. New API coming to get range value with its alternative description so I guess one may be used to get aria-valuenow and aria-valuetext and there are couple of more cool features.

[Radar WebKit Bug Importer]

<rdar://problem/16389406>

[Krzysztof Czech]

Created attachment 227663 [details]
patch

[Mario Sanchez Prada]

Comment on attachment 227663 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227663&action=review

Thanks for helping reduce the gap with latest ATK in WebKit! This patch looks good to me, I would just ask you to change a couple of things before landing (see them below)

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceValue.cpp:48
> +    gdouble value;

This is an internal variable so it can be a double

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceValue.cpp:78
> +    if (AccessibilityObject* coreObject = core(value)) {
> +        if (currentValue)
> +            *currentValue = coreObject->valueForRange();
> +        if (alternativeText)
> +            *alternativeText = g_strdup_printf("%s", coreObject->valueDescription().utf8().data());
> +    }

Could you switch this to an early return on !coreObject?

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceValue.cpp:88
> +    if (AccessibilityObject* coreObject = core(value))
> +        return webkitAccessibleGetIncrementValue(coreObject);
> +    return 0;

Ditto

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceValue.cpp:110
> +    if (AccessibilityObject* coreObject = core(value)) {
> +        gdouble minValue = coreObject->minValueForRange();
> +        gdouble maxValue = coreObject->maxValueForRange();
> +        gchar* valueDescription = g_strdup_printf("%s", coreObject->valueDescription().utf8().data());
> +        return atk_range_new(minValue, maxValue, valueDescription);
> +    }
> +    return nullptr;

Ditto

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:561
> +    if (AtkRange* range = atk_value_get_range(value)) {
> +        double value = isMinValue ? atk_range_get_lower_limit(range) : atk_range_get_upper_limit(range);
> +        atk_range_free(range);
> +        return value;
> +    }
> +    return 0;

Ditto

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:606
> +static double rangeMinMaxValue(AtkValue* value, bool isMinValue)

I don't like much bool parameters for these kind of uses. Would you mind defining a enum type instead and use that insteadl? Perhaps something such as enum RangeLimit { RangeLimitMinimum, RangeLimitMaximum } or the like.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:613
> +    if (AtkRange* range = atk_value_get_range(value)) {
> +        double value = isMinValue ? atk_range_get_lower_limit(range) : atk_range_get_upper_limit(range);
> +        atk_range_free(range);
> +        return value;
> +    }
> +    return 0;

Ditto

[Krzysztof Czech]

Created attachment 227751 [details]
attempt to check bots

[Krzysztof Czech]

Applied all Mario's suggestions.
Waiting whether bots are green.

[Krzysztof Czech]

Created attachment 227764 [details]
attempt to check bots

[Krzysztof Czech]

Created attachment 227836 [details]
attempt to check efl bots.

[WebKit Commit Bot]

Comment on attachment 227836 [details]
attempt to check efl bots.

Clearing flags on attachment: 227836

Committed r166288: <http://trac.webkit.org/changeset/166288>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.