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.

Created attachment 123474 [details] 1. Rename file for ATK AccessibilityObject wrapper

Created attachment 123475 [details] 2. Rename WebKitAccessible's public functions

Created attachment 123476 [details] 3. Fix typo in class struct and coding style in WebKitAccessibleHyperlink

Created attachment 123477 [details] 4. Move function returnString() to a new utility file

Created attachment 123478 [details] 5. New files for the implementation of the AtkAction interface

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.

Created attachment 123479 [details] 6. New files for the implementation of the AtkComponent interface

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 123481 [details] 4. Move function returnString() to a new utility file

Comment on attachment 123477 [details] 4. Move function returnString() to a new utility file I uploaded a newer version of this patch

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

Created attachment 123483 [details] 6. New files for the implementation of the AtkComponent 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 123484 [details] 7. New files for the implementation of the AtkDocument interface

Created attachment 123485 [details] 8. New files for the implementation of the AtkEditableText interface

Created attachment 123486 [details] 9. New files for the implementation of the AtkHyperlinkImpl interface

Created attachment 123487 [details] 10. New files for the implementation of the AtkHypertext interface

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

Created attachment 123491 [details] 11. New files for the implementation of the AtkImage interface

Created attachment 123492 [details] 12. New files for the implementation of the AtkSelection interface

Created attachment 123493 [details] 13. New files for the implementation of the AtkText interface

Created attachment 123494 [details] 14. New files for the implementation of the AtkTable interface

Created attachment 123495 [details] 15. New files for the implementation of the AtkValue interface

Created attachment 123496 [details] 16. Cleanup the list of includes in WebKitAccessibleWrapperAtk.cpp

Created attachment 123497 [details] 17. Fix coding style in the ATK AccessibilityObject wrapper

Committed r105604: <http://trac.webkit.org/changeset/105604>

Committed r105607: <http://trac.webkit.org/changeset/105607>

Committed r105608: <http://trac.webkit.org/changeset/105608>

Committed r105610: <http://trac.webkit.org/changeset/105610>

Committed r105618: <http://trac.webkit.org/changeset/105618>

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.

Committed r105716: <http://trac.webkit.org/changeset/105716>

Committed r105721: <http://trac.webkit.org/changeset/105721>

Committed r105722: <http://trac.webkit.org/changeset/105722>

Committed r105724: <http://trac.webkit.org/changeset/105724>

Committed r105725: <http://trac.webkit.org/changeset/105725>

Committed r105726: <http://trac.webkit.org/changeset/105726>

Committed r105727: <http://trac.webkit.org/changeset/105727>

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!

Committed r105740: <http://trac.webkit.org/changeset/105740>

(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!)

Committed r105745: <http://trac.webkit.org/changeset/105745>

(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 :-)

Committed r105749: <http://trac.webkit.org/changeset/105749>

Committed r105752: <http://trac.webkit.org/changeset/105752>

(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.

Committed r105754: <http://trac.webkit.org/changeset/105754>

Created attachment 123777 [details] 18. Don't expose functions for the ATK interfaces in header files

(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 on attachment 123777 [details] 18. Don't expose functions for the ATK interfaces in header files Thanks!

Committed r105791: <http://trac.webkit.org/changeset/105791>