[webkit-reviews] review granted: [Bug 130575] [ATK] Utilize new AtkValue interface coming in ATK 2.11.92 : [Attachment 227663] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Mar 25 06:39:17 PDT 2014


Mario Sanchez Prada <mario at webkit.org> has granted Krzysztof Czech
<k.czech at samsung.com>'s request for review:
Bug 130575: [ATK] Utilize new AtkValue interface coming in ATK 2.11.92
https://bugs.webkit.org/show_bug.cgi?id=130575

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

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
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


More information about the webkit-reviews mailing list