[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