Bug 76783 - [GTK] Refactor GTK's accessibilitity code to be more modular
Summary: [GTK] Refactor GTK's accessibilitity code to be more modular
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mario Sanchez Prada
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2012-01-21 08:51 PST by Mario Sanchez Prada
Modified: 2012-01-24 13:18 PST (History)
2 users (show)

See Also:


Attachments
Patch proposal (282.34 KB, patch)
2012-01-21 09:45 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
1. Rename file for ATK AccessibilityObject wrapper (215.91 KB, patch)
2012-01-22 08:53 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
2. Rename WebKitAccessible's public functions (8.20 KB, patch)
2012-01-22 08:56 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
3. Fix typo in class struct and coding style in WebKitAccessibleHyperlink (2.70 KB, patch)
2012-01-22 08:58 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
4. Move function returnString() to a new utility file (8.11 KB, patch)
2012-01-22 09:02 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
5. New files for the implementation of the AtkAction interface (10.85 KB, patch)
2012-01-22 09:04 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
6. New files for the implementation of the AtkComponent interface (14.95 KB, patch)
2012-01-22 09:08 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
4. Move function returnString() to a new utility file (8.06 KB, patch)
2012-01-22 09:56 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
6. New files for the implementation of the AtkComponent interface (16.55 KB, patch)
2012-01-22 10:07 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
6. New files for the implementation of the AtkComponent interface (14.95 KB, patch)
2012-01-22 10:11 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
7. New files for the implementation of the AtkDocument interface (15.01 KB, patch)
2012-01-22 10:18 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
8. New files for the implementation of the AtkEditableText interface (15.02 KB, patch)
2012-01-22 10:24 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
9. New files for the implementation of the AtkHyperlinkImpl interface (9.37 KB, patch)
2012-01-22 10:29 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
10. New files for the implementation of the AtkHypertext interface (12.60 KB, patch)
2012-01-22 10:32 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
10. New files for the implementation of the AtkHypertext interface (12.90 KB, patch)
2012-01-22 11:08 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
11. New files for the implementation of the AtkImage interface (10.29 KB, patch)
2012-01-22 11:10 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
12. New files for the implementation of the AtkSelection interface (23.28 KB, patch)
2012-01-22 11:12 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
13. New files for the implementation of the AtkText interface (77.25 KB, patch)
2012-01-22 11:14 PST, Mario Sanchez Prada
mrobinson: review-
Details | Formatted Diff | Diff
14. New files for the implementation of the AtkTable interface (24.72 KB, patch)
2012-01-22 11:16 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
15. New files for the implementation of the AtkValue interface (12.04 KB, patch)
2012-01-22 11:17 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
16. Cleanup the list of includes in WebKitAccessibleWrapperAtk.cpp (2.54 KB, patch)
2012-01-22 11:20 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
17. Fix coding style in the ATK AccessibilityObject wrapper (7.62 KB, patch)
2012-01-22 11:22 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
13. New files for the implementation of the AtkText interface (80.96 KB, patch)
2012-01-24 05:54 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
13. New files for the implementation of the AtkText interface (79.49 KB, patch)
2012-01-24 09:28 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff
18. Don't expose functions for the ATK interfaces in header files (62.93 KB, patch)
2012-01-24 11:30 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2012-01-21 08:51:37 PST
So far, the AccessibilityObject wrapper for ATK has been mainly implemented in terms of the following few files, inside WebCore/accessibility/gtk:

  - AccessibilityObjectWrapperAtk.cpp: 2780 lines
  - AccessibilityObjectWrapperAtk.h: 67 lines
  - WebKitAccessibleHyperlink.cpp: 408 lines
  - WebKitAccessibleHyperlink.h: 63 lines

As you can see, there's a BIG file which basically implements both all the functions for an AtkObject wrapping WebCore's AccessibilityObject *and* also all the ATK interfaces supported at the time by WebKitGTK+, which makes that file huge.

Then we have WebKitAccessibleHyperlink.[h|cpp] in separate files because it represents another kind of GObject (it's a subclass of AtkHyperlink, which is *not* and AtkObject), which has been created separately not to mix more things inside of that AccessibilityObjectWrapperAtk.cpp file.

The obvious problem with that huge file is that its destiny seems to be to keep growing out of control as long as we add more code, implement more interfaces, fix bugs... which doesn't look like a good idea from the point of view of readability and also because of more practical issues (not to mix so many unrelated things in the same file).

So, the proposal behind this bug is to take all the implementations for the different ATK interfaces out of that file (similarly to what Mozilla does), so we can keep all the related code, includes, internal functions... properly organized in different files. This way, we would leave in the wrapper's file just the implementation of AtkObject's functions and the code to decide which interfaces would implement each (dynamically generated) subclass of WebKitAccessible, and nothing else.

Also, bonus points for this approach is that it would give us a good opportunity to make all that code (2780) compliant with WebKit's coding style rules once and for all, which is a nice plus if you ask me :-)
Comment 1 Mario Sanchez Prada 2012-01-21 09:45:14 PST
Created attachment 123450 [details]
Patch proposal

Now attaching the patch to "fix" this "issue".

As you can see the patch is huge, and does not contains any new test. The reason for that, as you probably are already guessing, is that this patch doesn't change any behavior compared to what is in trunk at the time of writing this: it just moves code around to distribute it among different files, and rename some things to maatch WK's coding style.

I checked it several times locally and it keeps passing all the tests (both API and layout ones) so, combined with the fact that it didn't change anything, makes it a safe patch, I think.

So, now asking for review. Thank you in advance!

PS: It would be nice to get this patch (if approved) integrated soon, so other patches in the ATK code that might get in in the meanwhile don't make too hard to keep this big patch up-to-date.
Comment 2 Mario Sanchez Prada 2012-01-22 08:05:51 PST
Comment on attachment 123450 [details]
Patch proposal

Removing the r? flag, since I'll be now uploading a series of patches (which all will build and pass the tests at any step, if applied sequentially) for the same purpose.
Comment 3 Mario Sanchez Prada 2012-01-22 08:53:05 PST
Created attachment 123474 [details]
1. Rename file for ATK AccessibilityObject wrapper
Comment 4 Mario Sanchez Prada 2012-01-22 08:56:56 PST
Created attachment 123475 [details]
2. Rename WebKitAccessible's public functions
Comment 5 Mario Sanchez Prada 2012-01-22 08:58:38 PST
Created attachment 123476 [details]
3. Fix typo in class struct and coding style in WebKitAccessibleHyperlink
Comment 6 Mario Sanchez Prada 2012-01-22 09:02:35 PST
Created attachment 123477 [details]
4. Move function returnString() to a new utility file
Comment 7 Mario Sanchez Prada 2012-01-22 09:04:02 PST
Created attachment 123478 [details]
5. New files for the implementation of the AtkAction interface
Comment 8 Martin Robinson 2012-01-22 09:04:46 PST
Comment on attachment 123477 [details]
4. Move function returnString() to a new utility file

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

> Source/WebCore/ChangeLog:8
> +        Move common function returnString() our from the wrapper and

I think you have an extra "our" here.

> Source/WebCore/accessibility/gtk/WebKitAccessibleUtil.h:28
> +G_BEGIN_DECLS

No need for G_BEGIN_DECLS and G_END_DECLS here.
Comment 9 Mario Sanchez Prada 2012-01-22 09:08:07 PST
Created attachment 123479 [details]
6. New files for the implementation of the AtkComponent interface
Comment 10 Martin Robinson 2012-01-22 09:09:36 PST
Comment on attachment 123478 [details]
5. New files for the implementation of the AtkAction interface

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

Please fix the issues below.

> Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceAction.h:41
> +G_BEGIN_DECLS
> +
> +void webkitAccessibleActionInterfaceInit(AtkActionIface*);
> +
> +gboolean webkitAccessibleActionDoAction(AtkAction*, gint index);
> +
> +gint webkitAccessibleActionGetNActions(AtkAction*);
> +
> +const gchar* webkitAccessibleActionGetDescription(AtkAction*, gint index);
> +
> +const gchar* webkitAccessibleActionGetKeybinding(AtkAction*, gint index);
> +
> +const gchar* webkitAccessibleActionGetName(AtkAction*, gint index);
> +
> +G_END_DECLS

No need to put B_BEGIN_DECLS/G_END_DECLS. I don't think we usually separate function declarations by newlines like this.

> Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:2436
> +    {(GInterfaceInitFunc)webkitAccessibleActionInterfaceInit,

You should use a static cast here since you're rewriting the line.
Comment 11 Mario Sanchez Prada 2012-01-22 09:46:15 PST
(In reply to comment #10)
> [...]
> No need to put B_BEGIN_DECLS/G_END_DECLS. I don't think we usually separate function declarations by newlines like this.

Ok. Will fix it before landing.

> > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:2436
> > +    {(GInterfaceInitFunc)webkitAccessibleActionInterfaceInit,
> 
> You should use a static cast here since you're rewriting the line.

static_cast doesn't seem to work in this case, so I think the way to go is use reinterpret_cast instead. I know it's the most dangerous cast, but in this case is really equivalent to what we already have in place, so I think it's just fine.

Note: Before uploading more patches, I will update then with this suggestions. Thanks for the feedback!
Comment 12 Mario Sanchez Prada 2012-01-22 09:46:39 PST
(In reply to comment #8)
> (From update of attachment 123477 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123477&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Move common function returnString() our from the wrapper and
> 
> I think you have an extra "our" here.
> 
> > Source/WebCore/accessibility/gtk/WebKitAccessibleUtil.h:28
> > +G_BEGIN_DECLS
> 
> No need for G_BEGIN_DECLS and G_END_DECLS here.

Will fix this two as well before landing
Comment 13 Mario Sanchez Prada 2012-01-22 09:56:24 PST
Created attachment 123481 [details]
4. Move function returnString() to a new utility file
Comment 14 Mario Sanchez Prada 2012-01-22 09:57:03 PST
Comment on attachment 123477 [details]
4. Move function returnString() to a new utility file

I uploaded a newer version of this patch
Comment 15 Mario Sanchez Prada 2012-01-22 10:07:06 PST
Created attachment 123482 [details]
6. New files for the implementation of the AtkComponent interface

Uploading new version of this patch, taking into account the issues Martin pointed out for the one about the AtkAction interface
Comment 16 Mario Sanchez Prada 2012-01-22 10:11:45 PST
Created attachment 123483 [details]
6. New files for the implementation of the AtkComponent interface
Comment 17 Mario Sanchez Prada 2012-01-22 10:12:44 PST
Comment on attachment 123482 [details]
6. New files for the implementation of the AtkComponent interface

Looks like git format-patch messed up with this patch, so marking it as obsolete. Sorry for the spam
Comment 18 Mario Sanchez Prada 2012-01-22 10:18:40 PST
Created attachment 123484 [details]
7. New files for the implementation of the AtkDocument interface
Comment 19 Mario Sanchez Prada 2012-01-22 10:24:06 PST
Created attachment 123485 [details]
8. New files for the implementation of the AtkEditableText interface
Comment 20 Mario Sanchez Prada 2012-01-22 10:29:22 PST
Created attachment 123486 [details]
9. New files for the implementation of the AtkHyperlinkImpl interface
Comment 21 Mario Sanchez Prada 2012-01-22 10:32:13 PST
Created attachment 123487 [details]
10. New files for the implementation of the AtkHypertext interface
Comment 22 Mario Sanchez Prada 2012-01-22 11:08:02 PST
Created attachment 123490 [details]
10. New files for the implementation of the AtkHypertext interface

Attaching the patch again since the previous one missed the needed include line for WebkitAccessibleInterfaceHypertext.h in the ATK wrapper
Comment 23 Mario Sanchez Prada 2012-01-22 11:10:01 PST
Created attachment 123491 [details]
11. New files for the implementation of the AtkImage interface
Comment 24 Mario Sanchez Prada 2012-01-22 11:12:41 PST
Created attachment 123492 [details]
12. New files for the implementation of the AtkSelection interface
Comment 25 Mario Sanchez Prada 2012-01-22 11:14:08 PST
Created attachment 123493 [details]
13. New files for the implementation of the AtkText interface
Comment 26 Mario Sanchez Prada 2012-01-22 11:16:15 PST
Created attachment 123494 [details]
14. New files for the implementation of the AtkTable interface
Comment 27 Mario Sanchez Prada 2012-01-22 11:17:50 PST
Created attachment 123495 [details]
15. New files for the implementation of the AtkValue interface
Comment 28 Mario Sanchez Prada 2012-01-22 11:20:13 PST
Created attachment 123496 [details]
16. Cleanup the list of includes in WebKitAccessibleWrapperAtk.cpp
Comment 29 Mario Sanchez Prada 2012-01-22 11:22:32 PST
Created attachment 123497 [details]
17. Fix coding style in the ATK AccessibilityObject wrapper
Comment 30 Mario Sanchez Prada 2012-01-23 02:21:20 PST
Committed r105604: <http://trac.webkit.org/changeset/105604>
Comment 31 Mario Sanchez Prada 2012-01-23 03:43:45 PST
Committed r105607: <http://trac.webkit.org/changeset/105607>
Comment 32 Mario Sanchez Prada 2012-01-23 03:46:20 PST
Committed r105608: <http://trac.webkit.org/changeset/105608>
Comment 33 Mario Sanchez Prada 2012-01-23 03:55:17 PST
Committed r105610: <http://trac.webkit.org/changeset/105610>
Comment 34 Mario Sanchez Prada 2012-01-23 06:45:40 PST
Committed r105618: <http://trac.webkit.org/changeset/105618>
Comment 35 Martin Robinson 2012-01-23 10:04:34 PST
Comment on attachment 123483 [details]
6. New files for the implementation of the AtkComponent interface

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

Looks good, but consider the rename below.

> Source/WebCore/accessibility/gtk/WebKitAccessibleUtil.h:34
> +void contentsToAtk(WebCore::AccessibilityObject*, AtkCoordType, WebCore::IntRect, gint* x, gint* y, gint* width = 0, gint* height = 0);

If you need to put a comment explaining the method before the definition, it's a good sign that you should just rename the method to explain it.

contentsRelativetoAtkCoordinateType?
Comment 36 Martin Robinson 2012-01-23 10:06:00 PST
Comment on attachment 123484 [details]
7. New files for the implementation of the AtkDocument interface

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

> Source/WebCore/accessibility/gtk/WebKitAccessibleUtil.h:34
> +// Adds an accessibility attribute to an AtkAttributeSet, by name and value.
> +AtkAttributeSet* addAttributeToSet(AtkAttributeSet*, const char* name, const char* value);

Instead of having a comment explaining this function, ensure the function name is descriptive enough. In this case, it probably is and you can just omit the comment.
Comment 37 Martin Robinson 2012-01-23 10:09:36 PST
Comment on attachment 123490 [details]
10. New files for the implementation of the AtkHypertext interface

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

> Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceHypertext.h:28
> +AtkHyperlink* webkitAccessibleHypertextGetLink(AtkHypertext*, gint index);
> +gint webkitAccessibleHypertextGetNLinks(AtkHypertext*);
> +gint webkitAccessibleHypertextGetLinkIndex(AtkHypertext*, gint charIndex);

Are these methods used outside the file? If not they shouldn't be listed here and instead of should be declared at the top of the file and statically. This seems to be an issue with all of the patches. If you like, you can make this change after all the patches have landed.
Comment 38 Martin Robinson 2012-01-23 10:11:07 PST
Comment on attachment 123492 [details]
12. New files for the implementation of the AtkSelection interface

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

> Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceSelection.cpp:166
> +    return false;

It's a bit more correct to return FALSE or TRUE for functions that return gboolean, but this won't break anything.
Comment 39 Martin Robinson 2012-01-23 10:17:23 PST
Comment on attachment 123493 [details]
13. New files for the implementation of the AtkText interface

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

> Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:51
> +#include "htmlediting.h"
> +
> +#include <libgail-util/gail-util.h>
> +#include <pango/pango.h>

There is no newline needed between the includes here.

> Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:139
> +            gchar* text = convertUniCharToUTF8(renderText->characters(), renderText->textLength(), box->start(), box->end());

Wow! I think you could remove convertUniCharToUTF8 completely and just do:

String text = String(renderText->characters(), renderText->textLength());
g_string_append(text.substring(box->start(), box->end() - box->start()).utf8().data());

text is being leaked here anyway.

I know this is just code movement, but you might as well fix this one.
Comment 40 Martin Robinson 2012-01-23 10:18:41 PST
Comment on attachment 123496 [details]
16. Cleanup the list of includes in WebKitAccessibleWrapperAtk.cpp

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

Please fix the remaining style issue before landing.

> Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:-82
>  #include "visible_units.h"
>  
> -#include <atk/atk.h>
> -#include <glib.h>

There should be no extra new line here.
Comment 41 Mario Sanchez Prada 2012-01-24 03:04:08 PST
Committed r105716: <http://trac.webkit.org/changeset/105716>
Comment 42 Mario Sanchez Prada 2012-01-24 03:41:00 PST
Committed r105721: <http://trac.webkit.org/changeset/105721>
Comment 43 Mario Sanchez Prada 2012-01-24 03:48:09 PST
Committed r105722: <http://trac.webkit.org/changeset/105722>
Comment 44 Mario Sanchez Prada 2012-01-24 03:55:34 PST
Committed r105724: <http://trac.webkit.org/changeset/105724>
Comment 45 Mario Sanchez Prada 2012-01-24 03:59:08 PST
Committed r105725: <http://trac.webkit.org/changeset/105725>
Comment 46 Mario Sanchez Prada 2012-01-24 04:04:35 PST
Committed r105726: <http://trac.webkit.org/changeset/105726>
Comment 47 Mario Sanchez Prada 2012-01-24 04:25:30 PST
Committed r105727: <http://trac.webkit.org/changeset/105727>
Comment 48 Mario Sanchez Prada 2012-01-24 05:54:14 PST
Created attachment 123720 [details]
13. New files for the implementation of the AtkText interface

(In reply to comment #39)
> [...]
> > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:51
> > +#include "htmlediting.h"
> > +
> > +#include <libgail-util/gail-util.h>
> > +#include <pango/pango.h>
> 
> There is no newline needed between the includes here.

Ok. Fixed.

> > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:139
> > +            gchar* text = convertUniCharToUTF8(renderText->characters(), renderText->textLength(), box->start(), box->end());
> 
> Wow! I think you could remove convertUniCharToUTF8 completely and just do:
> 
> String text = String(renderText->characters(), renderText->textLength());
> g_string_append(text.substring(box->start(), box->end() - box->start()).utf8().data());

Hmm... I'm afraid that wouldn't work fine in this specific case, since convertUniCharToUTF8() does some more -a11y specific- stuff than what the String class provides, such as replacing breaklines with blank spaces, so the text exposed through the AtkText interface can be seen as a single string for multiline paragraphs.

However, perhaps this is something that could be fixed/improved without needing this (strange?) function, but at the time of this refactoring I'd rather leave it as it is, since that way all the tests keep passing, which is something that doesn't happen if I use the two lines you proposed here (some API tests will fail).

> text is being leaked here anyway.

Good point. I already fixed this in this new patch, by using GOwnPtr.

> I know this is just code movement, but you might as well fix this one.

I fixed the extra line and the leak, but left the call to convertUniCharToUTF8() in the patch since, as I mentioned above, using your suggestion will break the tests, and so perhaps such a change would deserve a different bug to treat it.
Comment 49 Mario Sanchez Prada 2012-01-24 07:36:53 PST
[Replying to two comments at once]

(In reply to comment #37)
> (From update of attachment 123490 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123490&action=review
> 
> > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceHypertext.h:28
> > +AtkHyperlink* webkitAccessibleHypertextGetLink(AtkHypertext*, gint index);
> > +gint webkitAccessibleHypertextGetNLinks(AtkHypertext*);
> > +gint webkitAccessibleHypertextGetLinkIndex(AtkHypertext*, gint charIndex);
> 
> Are these methods used outside the file? If not they shouldn't be listed here and instead of should be 
> declared at the top of the file and statically. This seems to be an issue with all of the patches. If you like, 
> you can make this change after all the patches have landed.

The thing is that sometimes it could be interesting to call to some of those functions from the wrapper or even from the implementation files for other ATK interfaces (e.g. Some functions from WebKitInterfaceText are used from WebKitInterfaceTable.cpp), so that's why I thought it would be interesting to make this functions in the header files.

Another option would be to just make public the WebKitAccessibleXYZInit() function in there and using the generic ATK functions for the interfaces where needed, instead of the specific functions implementing those interfaces in WebKitGTK, but I somewhat I'm inclined to think that the second approach is better since, at the end, even if we use for instance atk_text_get_text() in WebKitAccessibleInterfaceTable.cpp (instead of webkitAccessibleTextGetText, as available in WebKitAccessibleInterfaceText.h), the AtkText object we pass to it must be an instance of WebKitAccessible (subclass of AtkObject) implementing the AtkText interface. And it turns out that whenever we need to call functions of one interface from another interface's implementation (or the wrapper's implementation), as we are always inside WebKitGTK, those objects will always be instances of WebKitAccessible, so why to do the extra roundtrip of calling to more generic functions (e.g. AtkText's functions) instead of using those already provided by our custom implementation for those ATK interfaces (e.g. WebKitAccessibleInterfaceText) ? 

I would understand reasons based on "keeping it more abstract", but at the end you need that the AtkObjects passed to them are actually instances of our wrapper (otherwise they will fail to find the WebCore's a11y object they rely on), and so I only need additional an unnecesary levels of indirection in calling, for instance, atk_text_get_text() instead of directly. webkitAccessibleTextGetText.

So, if we agree on this, I think it's better to publish all the functions related to the implementation of the API for each ATK interface in the header files. If, for the contrary, we finally agree it's better to use more generic functions, we would only publish the WebKitAccessibleXYZInterfaceInit() functions and change calls as needed. But I'd rather track that as a separate bug in any case.

Opinions? :)

(In reply to comment #38)
> (From update of attachment 123492 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123492&action=review
> 
> > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceSelection.cpp:166
> > +    return false;
> 
> It's a bit more correct to return FALSE or TRUE for functions that return gboolean, but this 
> won't break anything.

I changed that before landing the patch.

Thanks for the feedback!
Comment 50 Mario Sanchez Prada 2012-01-24 08:23:15 PST
Committed r105740: <http://trac.webkit.org/changeset/105740>
Comment 51 Martin Robinson 2012-01-24 08:39:16 PST
(In reply to comment #49)
> Another option would be to just make public the WebKitAccessibleXYZInit() function in there and using the generic ATK functions for the interfaces where needed, instead of the specific functions implementing those interfaces in WebKitGTK, but I somewhat I'm inclined to think that the second approach is better since, at the end, even if we use for instance atk_text_get_text() in WebKitAccessibleInterfaceTable.cpp (instead of webkitAccessibleTextGetText, as available in WebKitAccessibleInterfaceText.h), the AtkText object we pass to it must be an instance of WebKitAccessible (subclass of AtkObject) implementing the AtkText interface. And it turns out that whenever we need to call functions of one interface from another interface's implementation (or the wrapper's implementation), as we are always inside WebKitGTK, those objects will always be instances of WebKitAccessible, so why to do the extra roundtrip of calling to more generic functions (e.g. AtkText's functions) instead of using those already provided by our custom implementation for those ATK interfaces (e.g. WebKitAccessibleInterfaceText) ? 

I don't have a strong opinion either way. I do think that if the method isn't called outside the file though, it shouldn't be exported in the header. Why not just export the ones that you do use outside the file?
Comment 52 Martin Robinson 2012-01-24 08:44:49 PST
Comment on attachment 123720 [details]
13. New files for the implementation of the AtkText interface

Thanks for fixing the leak. Mario has agreed to try to replace the use of Pango with a followup patch.
Comment 53 Mario Sanchez Prada 2012-01-24 09:28:58 PST
Created attachment 123750 [details]
13. New files for the implementation of the AtkText interface

(In reply to comment #48)
> [...]> > > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:139
> > > +            gchar* text = convertUniCharToUTF8(renderText->characters(), renderText->textLength(), box->start(), box->end());
> > 
> > Wow! I think you could remove convertUniCharToUTF8 completely and just do:
> > 
> > String text = String(renderText->characters(), renderText->textLength());
> > g_string_append(text.substring(box->start(), box->end() - box->start()).utf8().data());
> 
> Hmm... I'm afraid that wouldn't work fine in this specific case, since convertUniCharToUTF8() does some more -a11y specific- stuff than what the String class provides, such as replacing breaklines with blank spaces, so the text exposed through the AtkText interface can be seen as a single string for multiline paragraphs.
> 
> However, perhaps this is something that could be fixed/improved without needing this (strange?) function, but at the time of this refactoring I'd rather leave it as it is, since that way all the tests keep passing, which is something that doesn't happen if I use the two lines you proposed here (some API tests will fail).

You were right: it was possible to get rid of convertUniCharToUTF8(), so now uploding a new patch that also contains that change (and still passes the tests!)
Comment 54 Mario Sanchez Prada 2012-01-24 10:02:26 PST
Committed r105745: <http://trac.webkit.org/changeset/105745>
Comment 55 Mario Sanchez Prada 2012-01-24 10:04:31 PST
(In reply to comment #51)
> [...]
> I don't have a strong opinion either way. I do think that if the method isn't called outside the file though, it shouldn't be exported in the header. Why not just export the ones that you do use outside the file?

I just think that it be nice to keep them all of them exposed in the header files so they are already there if needed from somewhere inside accessibility/gtk. That is, I see it as an internal library for being used by WebKitGTK a11y code :-)
Comment 56 Mario Sanchez Prada 2012-01-24 10:28:38 PST
Committed r105749: <http://trac.webkit.org/changeset/105749>
Comment 57 Mario Sanchez Prada 2012-01-24 10:38:01 PST
Committed r105752: <http://trac.webkit.org/changeset/105752>
Comment 58 Martin Robinson 2012-01-24 10:40:28 PST
(In reply to comment #55)

> I just think that it be nice to keep them all of them exposed in the header files so they are already there if needed from somewhere inside accessibility/gtk. That is, I see it as an internal library for being used by WebKitGTK a11y code :-)

Think of it this way: when something is static to a file you know for certain that it's fine to change the behavior of something or remove it. Exporting all the methods unconditionally is like making all members public in an object.
Comment 59 Mario Sanchez Prada 2012-01-24 10:49:08 PST
Committed r105754: <http://trac.webkit.org/changeset/105754>
Comment 60 Mario Sanchez Prada 2012-01-24 11:30:15 PST
Created attachment 123777 [details]
18. Don't expose functions for the ATK interfaces in header files
Comment 61 Mario Sanchez Prada 2012-01-24 11:31:41 PST
(In reply to comment #58)
> (In reply to comment #55)
> 
> > I just think that it be nice to keep them all of them exposed in the header files so they are already there if needed from somewhere inside accessibility/gtk. That is, I see it as an internal library for being used by WebKitGTK a11y code :-)
> 
> Think of it this way: when something is static to a file you know for certain that it's fine to change the behavior of something or remove it. Exporting all the methods unconditionally is like making all members public in an object.

As you can see in comment #60, I just added a last patch to this bug to fix that. 

So yes... you "won" :-)
Comment 62 Martin Robinson 2012-01-24 11:34:48 PST
Comment on attachment 123777 [details]
18. Don't expose functions for the ATK interfaces in header files

Thanks!
Comment 63 Mario Sanchez Prada 2012-01-24 13:18:54 PST
Committed r105791: <http://trac.webkit.org/changeset/105791>