[webkit-reviews] review requested: [Bug 76783] [GTK] Refactor GTK's accessibilitity code to be more modular : [Attachment 123720] 13. New files for the implementation of the AtkText interface

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 76783: [GTK] Refactor GTK's accessibilitity code to be more modular
https://bugs.webkit.org/show_bug.cgi?id=76783

Attachment 123720: 13. New files for the implementation of the AtkText
interface
https://bugs.webkit.org/attachment.cgi?id=123720&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
(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.