Bug 115647

Summary: [atk] Replace deprecated call to atk_document_get_locale() in DumpRenderTree
Product: WebKit Reporter: Eduardo Lima Mitev <elima>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, berto, buildbot, cfleizach, cgarcia, commit-queue, darin, dmazzoni, eflews.bot, gyuyoung.kim, jdiggs, k.czech, mario, mrobinson, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 116726    
Bug Blocks:    
Attachments:
Description Flags
patch
eflews.bot: commit-queue-
new patch moving implementations to AtkObject::get_object_locale()
eflews.bot: commit-queue-
updated patch
none
last patch with core style fixes
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
updated patch
mrobinson: review-
patch reworked to remove GOwnPtr on textAttributes none

Description Eduardo Lima Mitev 2013-05-06 08:07:36 PDT
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.
Comment 1 Eduardo Lima Mitev 2013-05-06 08:12:17 PDT
Created attachment 200672 [details]
patch
Comment 2 Eduardo Lima Mitev 2013-05-06 08:27:56 PDT
adding Alejandro Piñeiro and Carlos García to CC
Comment 3 EFL EWS Bot 2013-05-06 08:36:01 PDT
Comment on attachment 200672 [details]
patch

Attachment 200672 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/325059
Comment 4 Eduardo Lima Mitev 2013-05-06 09:34:47 PDT
gyuyoung.kim, do you know if there are plans to upgrade ATK from 2.4.0 to 2.7.90+ in EFL EWS?

cheers
Comment 5 Alejandro Piñeiro 2013-05-06 10:17:14 PDT
(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
Comment 6 Martin Robinson 2013-05-06 10:19:18 PDT
(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.
Comment 7 Alejandro Piñeiro 2013-05-06 10:33:35 PDT
(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
Comment 8 Eduardo Lima Mitev 2013-05-06 10:35:07 PDT
Ok, all issues noted. I will rewrite the patch addressing those.

Thanks a lot for your feedback.
Comment 9 Eduardo Lima Mitev 2013-05-06 11:04:06 PDT
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?
Comment 10 Mario Sanchez Prada 2013-05-07 07:39:54 PDT
(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 :-)
Comment 11 Eduardo Lima Mitev 2013-05-13 07:16:26 PDT
Created attachment 201562 [details]
new patch moving implementations to AtkObject::get_object_locale()
Comment 12 EFL EWS Bot 2013-05-13 08:09:23 PDT
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
Comment 13 EFL EWS Bot 2013-05-13 08:29:48 PDT
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
Comment 14 Eduardo Lima Mitev 2013-05-24 10:11:13 PDT
Adding dependency on bug 116726.
Comment 15 Martin Robinson 2013-05-24 10:15:50 PDT
Comment on attachment 201562 [details]
new patch moving implementations to AtkObject::get_object_locale()

Would love to have Mario look at this one.
Comment 16 Mario Sanchez Prada 2013-05-28 01:56:30 PDT
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
Comment 17 Mario Sanchez Prada 2013-05-28 02:12:48 PDT
(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
Comment 18 Eduardo Lima Mitev 2013-05-28 08:09:35 PDT
(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
Comment 19 Alberto Garcia 2013-05-30 02:19:17 PDT
(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
Comment 20 Eduardo Lima Mitev 2013-05-30 02:30:44 PDT
(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.
Comment 21 Alejandro Piñeiro 2013-05-30 03:01:16 PDT
(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).
Comment 22 Alberto Garcia 2013-05-30 03:04:52 PDT
(In reply to comment #21)
> But the fact is that the dependency to ATK was already bumped.

Then it's fine :)
Comment 23 Eduardo Lima Mitev 2013-05-30 03:56:16 PDT
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.
Comment 24 Mario Sanchez Prada 2013-05-30 05:29:22 PDT
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... :)
Comment 25 Eduardo Lima Mitev 2013-05-30 06:03:54 PDT
Created attachment 203346 [details]
last patch with core style fixes
Comment 26 Eduardo Lima Mitev 2013-05-30 09:28:40 PDT
> last patch with core style fixes
                  ----
                  code
Comment 27 Build Bot 2013-05-30 10:48:28 PDT
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
Comment 28 Build Bot 2013-05-30 10:48:31 PDT
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
Comment 29 Eduardo Lima Mitev 2013-06-03 10:29:04 PDT
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().
Comment 30 Martin Robinson 2013-06-03 13:29:42 PDT
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.
Comment 31 Mario Sanchez Prada 2013-06-04 01:41:44 PDT
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).
Comment 32 Eduardo Lima Mitev 2013-06-04 01:50:13 PDT
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.
Comment 33 Eduardo Lima Mitev 2013-06-05 12:39:37 PDT
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`.
Comment 34 WebKit Commit Bot 2013-06-12 14:39:51 PDT
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>
Comment 35 WebKit Commit Bot 2013-06-12 14:39:56 PDT
All reviewed patches have been landed.  Closing bug.