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 :-)
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 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 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 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.
(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!
(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
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 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
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 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 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 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 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 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 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.
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.
[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!
(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 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.
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!)
(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 :-)
(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.
(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" :-)
2012-01-21 09:45 PST, Mario Sanchez Prada
2012-01-22 08:53 PST, Mario Sanchez Prada
2012-01-22 08:56 PST, Mario Sanchez Prada
2012-01-22 08:58 PST, Mario Sanchez Prada
2012-01-22 09:02 PST, Mario Sanchez Prada
2012-01-22 09:04 PST, Mario Sanchez Prada
2012-01-22 09:08 PST, Mario Sanchez Prada
2012-01-22 09:56 PST, Mario Sanchez Prada
2012-01-22 10:07 PST, Mario Sanchez Prada
2012-01-22 10:11 PST, Mario Sanchez Prada
2012-01-22 10:18 PST, Mario Sanchez Prada
2012-01-22 10:24 PST, Mario Sanchez Prada
2012-01-22 10:29 PST, Mario Sanchez Prada
2012-01-22 10:32 PST, Mario Sanchez Prada
2012-01-22 11:08 PST, Mario Sanchez Prada
2012-01-22 11:10 PST, Mario Sanchez Prada
2012-01-22 11:12 PST, Mario Sanchez Prada
2012-01-22 11:14 PST, Mario Sanchez Prada
2012-01-22 11:16 PST, Mario Sanchez Prada
2012-01-22 11:17 PST, Mario Sanchez Prada
2012-01-22 11:20 PST, Mario Sanchez Prada
2012-01-22 11:22 PST, Mario Sanchez Prada
2012-01-24 05:54 PST, Mario Sanchez Prada
2012-01-24 09:28 PST, Mario Sanchez Prada
2012-01-24 11:30 PST, Mario Sanchez Prada