Bug 115647 – [atk] Replace deprecated call to atk_document_get_locale() in DumpRenderTree

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eduardo Lima Mitev]

DumpRenderTree accessibility code calls atk_document_get_locale(), which has been deprecated since ATK 2.7.90. atk_object_get_object_locale() should be used instead.

[Eduardo Lima Mitev]

Created attachment 200672 [details]
patch

[Eduardo Lima Mitev]

adding Alejandro Piñeiro and Carlos García to CC

[EFL EWS Bot]

Comment on attachment 200672 [details]
patch

Attachment 200672 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/325059

[Eduardo Lima Mitev]

gyuyoung.kim, do you know if there are plans to upgrade ATK from 2.4.0 to 2.7.90+ in EFL EWS?

cheers

[Alejandro Piñeiro]

(In reply to comment #2)
> adding Alejandro Piñeiro and Carlos García to CC

Note that I'm not a reviewer (or a committer in any case), so consider this as an informal review.

Note that the call to the deprecated atk_document_get_locale is done on a if. And the condition is if the object is a document itself (so ensuring that the call will be correct).

So if we replace atk_document_get_locale (document specific) for a more general purpose atk_object_get_object_locale, that if condition doesn't have sense anymore.

In general, I really think that the the implementation of dumprendertree::language should be replaced atk_object_get_object_locale, as is the purpose of both methods are the same.

Just in case someone wonders if the attribute ATK_TEXT_ATTR_LANGUAGE should be deprecated: it is still needed because you can fine-grained requesting the language, as atk_text_get_run_attributes [1] allows you to request the atributtes (so also language) of a specific text range. If having more than one language for each text object is possible depends on the implementor.

Also note that if we replace the implementation of ::language with just a call to atk_object_get_object_locale, probably we will get into a regression, as atk_object_get_object_locale is not implemented at all at webkit atk implementation. 


[1] https://developer.gnome.org/atk/stable/AtkText.html#atk-text-get-run-attributes

[Martin Robinson]

(In reply to comment #5)

> Note that the call to the deprecated atk_document_get_locale is done on a if. And the condition is if the object is a document itself (so ensuring that the call will be correct).

Seems like a very reasonable concern.

[Alejandro Piñeiro]

(In reply to comment #5)

> 
> Also note that if we replace the implementation of ::language with just a call to atk_object_get_object_locale, probably we will get into a regression, as atk_object_get_object_locale is not implemented at all at webkit atk implementation. 

And btw, there is an already existing implementation of atk_document_get_locale:

http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceDocument.cpp#L112

So two things:

  1. This implementation is not needed anymore, because the method is deprecated.
  2. It is needed an implementation for atk_object_get_object_locale, to avoid the regression I'm mentioning

[Eduardo Lima Mitev]

Ok, all issues noted. I will rewrite the patch addressing those.

Thanks a lot for your feedback.

[Eduardo Lima Mitev]

After discussing this with Alejandro (ATK maintainer), my proposal is to:

1) move the current implementation of get_locale() from AtkDocument interface, to AtkObject get_object_locale() virtual method. With this, it is safe to replace atk_document_get_locale() by atk_object_get_object_locale() for document objects.

2) take the logic used in AccessibilityUIElement::language() to get the locale for AtkText objects, and move it to get_object_locale() virtual method of AtkObject. With this, we can call atk_object_get_object_locale() on AtkText objects as well, we clean up the language() method, and improve encapsulation.

Alejandro, Martin, do you agree with this plan?

[Mario Sanchez Prada]

(In reply to comment #9)
> After discussing this with Alejandro (ATK maintainer), my proposal is to:
> 
> 1) move the current implementation of get_locale() from AtkDocument interface, to AtkObject get_object_locale() virtual method. With this, it is safe to replace atk_document_get_locale() by atk_object_get_object_locale() for document objects.
> 
> 2) take the logic used in AccessibilityUIElement::language() to get the locale for AtkText objects, and move it to get_object_locale() virtual method of AtkObject. With this, we can call atk_object_get_object_locale() on AtkText objects as well, we clean up the language() method, and improve encapsulation.
> 
> Alejandro, Martin, do you agree with this plan?

Even I'm neither Alejandro nor Martin I would like to say I agree with this plan too :-)

[Eduardo Lima Mitev]

Created attachment 201562 [details]
new patch moving implementations to AtkObject::get_object_locale()

[EFL EWS Bot]

Comment on attachment 201562 [details]
new patch moving implementations to AtkObject::get_object_locale()

Attachment 201562 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/466285

[EFL EWS Bot]

Comment on attachment 201562 [details]
new patch moving implementations to AtkObject::get_object_locale()

Attachment 201562 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/346816

[Eduardo Lima Mitev]

Adding dependency on bug 116726.

[Martin Robinson]

Comment on attachment 201562 [details]
new patch moving implementations to AtkObject::get_object_locale()

Would love to have Mario look at this one.

[Mario Sanchez Prada]

Comment on attachment 201562 [details]
new patch moving implementations to AtkObject::get_object_locale()

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

[Mario Sanchez Prada]

(In reply to comment #16)
> [...]
> 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)
       ^^^
I obviously meant "file" here :).

Anyway, just realized it doesn't make sense to file a new bug here since you are _removing_ that code from DRT and making it depend on the new method atk_object_get_object_locale(), so I won't file anything.

What I would make sense though, would be for you to also update the implementation in WebKitTestRunner's AccessibilityUIElement::language to match the one you are setting now in the DRT, so you have the correct non-leaking code in the wrapper and you just use it from DRT and WKTR.

Could you make that change too in this patch?

Note: Adding Krzysztof Czech on CC to keep him on the loop

[Eduardo Lima Mitev]

(In reply to comment #17)
> 
> Could you make that change too in this patch?
> 

Thanks a lot for the feedback! Sure, I will update the patch following your comments.

cheers

[Alberto Garcia]

(In reply to comment #0)
> DumpRenderTree accessibility code calls atk_document_get_locale(),
> which has been deprecated since ATK
> 2.7.90. atk_object_get_object_locale() should be used instead.

Sorry, I'm late for this.

In general I think we should be careful with making changes like this
if there's no good reason.

You're bumping the ATK dependency to >= 2.7.90 without fixing any bugs
or adding new functionality, which restricts the number of potential
users:

http://packages.ubuntu.com/search?keywords=libatk1.0-0

http://packages.debian.org/libatk1.0-0

https://apps.fedoraproject.org/packages/atk

[Eduardo Lima Mitev]

(In reply to comment #19)
> You're bumping the ATK dependency to >= 2.7.90 without fixing any bugs
> or adding new functionality, which restricts the number of potential
> users:
>

I understand that, but right now the fix of this patch is becoming more a code cleaning issue. We are centralizing the implementation of language() methods from DumpRenderTree and WebKitTestRunner, into WebCore, using the AtkObject::get_object_locale(), provided for that specific purpose. Right now that implementation is basically duplicated, and poorly encapsulated.

Also, there is a memory leak in the DumpRenderTree version of language(), that I'm now fixing as part of moving the implementation to WebCore.

[Alejandro Piñeiro]

(In reply to comment #19)
> (In reply to comment #0)
> > DumpRenderTree accessibility code calls atk_document_get_locale(),
> > which has been deprecated since ATK
> > 2.7.90. atk_object_get_object_locale() should be used instead.
> 
> Sorry, I'm late for this.
> 
> In general I think we should be careful with making changes like this
> if there's no good reason.
> 
> You're bumping the ATK dependency to >= 2.7.90 without fixing any bugs
> or adding new functionality, which restricts the number of potential
> users:
> 
> http://packages.ubuntu.com/search?keywords=libatk1.0-0
> 
> http://packages.debian.org/libatk1.0-0
> 
> https://apps.fedoraproject.org/packages/atk

But the fact is that the dependency to ATK was already bumped. Take a look to bug 98376 comment 6. We bumped ATK dependency in order to fix a bug (detected by a non-passing test), so we added new functionality.

After some weeks, it was decided to bumpt to 2.8.0 (a stable release). This bump was managed on bug 113883.

So since two months now, the ATK support on WebKit needs ATK 2.8.0.

The problem here is that the EFL bots didn't update that (in relation with bug 98376, I guess that they are still skipping it).

[Alberto Garcia]

(In reply to comment #21)
> But the fact is that the dependency to ATK was already bumped.

Then it's fine :)

[Eduardo Lima Mitev]

Created attachment 203340 [details]
updated patch

This new patch addresses the issues commented by Mario.

As an implementation note, I reused AtkCachedDocumentLocale in WebKitAccessibleInterfaceDocument::webkitAccessibleDocumentGetLocale() for both AtkDocument and AtkText.

[Mario Sanchez Prada]

Comment on attachment 203340 [details]
updated patch

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

Patch looks good to me. I just found a couple of minor nits that I've commented about below...

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:813
> +        if (!core(object))
> +            return 0;
> +
> +        // TODO: Should we fall back on lang xml:lang when the following comes up empty?
> +        String language = core(object)->language();

Small nit: you can have a coreObject variable and only call core(object) once here.

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:818
> +        const gchar *locale = 0;

Misplaced *: gchar * -> gchar*

Also, I think the current style rules prefer char* to gchar* for non-public code.

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:435
> +    const gchar *locale = atk_object_get_object_locale(ATK_OBJECT(m_element));

Same considerations than above

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:593
> +    const gchar *locale = atk_object_get_object_locale(ATK_OBJECT(m_element.get()));

And yet again... :)

[Eduardo Lima Mitev]

Created attachment 203346 [details]
last patch with core style fixes

[Eduardo Lima Mitev]

> last patch with core style fixes
                  ----
                  code

[Build Bot]

Comment on attachment 203346 [details]
last patch with core style fixes

Attachment 203346 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/656594

New failing tests:
fast/dom/location-new-window-no-crash.html

[Build Bot]

Created attachment 203366 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Eduardo Lima Mitev]

Created attachment 203613 [details]
updated patch

This is basically previous patch but using atk_attribute_set_free() instead of custom attributesClear() function, to free the AtkAttributeSet returned by atk_text_get_default_attributes().

[Martin Robinson]

Comment on attachment 203613 [details]
updated patch

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

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:820
> +        atk_attribute_set_free(textAttributes.get());

I think it's a mistake to use a GOwnPtr and to manually free the same thing. Either make GOwnPtr specialization for AtkTextAttributes (if one doesn't exist already) or free it manually.

[Mario Sanchez Prada]

Comment on attachment 203613 [details]
updated patch

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

>> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:820
>> +        atk_attribute_set_free(textAttributes.get());
> 
> I think it's a mistake to use a GOwnPtr and to manually free the same thing. Either make GOwnPtr specialization for AtkTextAttributes (if one doesn't exist already) or free it manually.

I agree with Martin here. If you use atk_attribute_set_free() then you should not use GOwnPtr at all, since you might be causing a double-free error later: atk_attribute_set_free() frees the memory used by the GSList but does NOT set that pointer to NULL, so when you go out of scope the GOwnPtr will try to free again the already freed memory block.

About the idea of specializing GOwnPtr for AtkAttributeSet, I'm fine with that too, but I guess it would be better to get this patch landed first and then file a new bug to "fix" the memory freeing logic at once with one patch, doing the needed changes where needed (I think it would be DRT, WKTR and this wrapper only, but please double check).

[Eduardo Lima Mitev]

I see your points guys, ok. Then patch from comment 25 should be ok to merge. It is essentially the same but using the attributesClear() helper, as it was.

[Eduardo Lima Mitev]

Created attachment 203873 [details]
patch reworked to remove GOwnPtr on textAttributes

This new patch removes the GOwnPtr wrapper and use the AtkAttributeSet pointer directly.

Following Mario's comment, I think we might want to consider specializing GOwnPtr for AtkAttributeSet, but as part of a new story, and move on with this patch.

The only other place where a GOwnPtr is used on an AtkAttributeSet is in `Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:68`.

[WebKit Commit Bot]

Comment on attachment 203873 [details]
patch reworked to remove GOwnPtr on textAttributes

Clearing flags on attachment: 203873

Committed r151524: <http://trac.webkit.org/changeset/151524>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.