[webkit-reviews] review granted: [Bug 25415] [GTK][ATK] Please implement support for get_text_at_offset : [Attachment 32451] lineboundary.patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted Xan Lopez
<xan.lopez at gmail.com>'s request for review:
Bug 25415: [GTK][ATK] Please implement support for get_text_at_offset
https://bugs.webkit.org/show_bug.cgi?id=25415

Attachment 32451: lineboundary.patch
https://bugs.webkit.org/attachment.cgi?id=32451&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
> +static void utf16_to_utf8(const UChar* aText, gint aLength, char* &text,
gint &length)
> +{
> +    gboolean need_copy = FALSE;

I believe we want UTF16ToUTF8, and needCopy, here, though the first one looks
blergh. Maybe Utf16ToUtf8? Though the style doesn't like that for acronyms...

> +	   if (!aText[i] || IS_LOW_SURROGATE(aText[i])) {
> +	       need_copy = TRUE;
> +	       break;
> +	   }
> +	   else if (IS_HIGH_SURROGATE(aText[i])) {

The else if line should be after the if's } here.

> +    gint new_length = 0;

newLength

It would be good to have a comment on top of convertUniCharToUTF8 explaining
that it is not really general, as the name seems to imply, and that it is
concerned with how the layouting is done. I know there's a comment that may
lead people to notice this in the middle of the function, but then again, it
would be best to make this obvious.

The rest looks fine to me, so r=me with the style changes. It's a pitty you had
to do all that messing with the text to get it right, but I figure it would be
much harder to change the way webcore works.