[Webkit-unassigned] [Bug 115647] [atk] Replace deprecated call to atk_document_get_locale() in DumpRenderTree

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=115647





--- Comment #16 from Mario Sanchez Prada <mario at webkit.org>  2013-05-28 01:56:30 PST ---
(From update of attachment 201562)
View in context: https://bugs.webkit.org/attachment.cgi?id=201562&action=review

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceDocument.cpp:-112
> -static const gchar* webkitAccessibleDocumentGetLocale(AtkDocument* document)
> -{
> -    // TODO: Should we fall back on lang xml:lang when the following comes up empty?
> -    String language = core(document)->language();
> -    if (!language.isEmpty())
> -        return cacheAndReturnAtkProperty(ATK_OBJECT(document), AtkCachedDocumentLocale, language);
> -
> -    return 0;
> -}
> -
>  void webkitAccessibleDocumentInterfaceInit(AtkDocumentIface* iface)
>  {
>      iface->get_document_attribute_value = webkitAccessibleDocumentGetAttributeValue;
>      iface->get_document_attributes = webkitAccessibleDocumentGetAttributes;
> -    iface->get_document_locale = webkitAccessibleDocumentGetLocale;

Even if atk_document_get_document_locale() method is deprecated, I think it would be better not to completely remove the method here, to avoid clients that might be using this functionality from breaking.

What about replacing the implementation of webkitAccessibleDocumentGetLocale() with a simple call to atk_object_get_object_locale() instead? That would use the new implementation, while not break the former users of the API.

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:799
> +        String language = core(object)->language();

I know it's not commont it happens and also that it was not in the original code you were moving, but I think a extra null check (plus early return if needed) for coreObject here might be a good thing, before calling the language() method.

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:808
> +        for (GSList* textAttributes = atk_text_get_default_attributes(ATK_TEXT(object)); textAttributes; textAttributes = textAttributes->next) {
> +            AtkAttribute* atkAttribute = static_cast<AtkAttribute*>(textAttributes->data);
> +            if (!strcmp(atkAttribute->name, atk_text_attribute_get_name(ATK_TEXT_ATTR_LANGUAGE)))
> +                return atkAttribute->value;
> +        }

atk_text_get_default_attributes() returns a transfer-full GSList of structs filled with char* pointers, meaning that you should free both the memory for those structs and for the GSList before returning from thie method, otherwise you're having a pretty nice leak here. You can check the implementation of AccessibilityUIElement::language in WebKitTestRunner for an already fixed version of the same logic (actually, I think it was already fixed for DRT too. I'll fix a bug about that)

In any case, it is not your fault as you're just moving code here, but anyway you should get the right code before committing anything, since this is not DRT (where the leak comes from) and so a leak like that in here will have a more severe impact, so please fix this before landing anything.

> Tools/ChangeLog:8
> +        After moving the resoluton of the AtkObject locale to

resoluton -> resolution

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.