[webkit-reviews] review granted: [Bug 120638] [ATK] Implement new API in AtkText: atk_text_get_string_at_offset() : [Attachment 210383] Patch proposal plus new Unit tests

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted Mario Sanchez Prada
<mario at webkit.org>'s request for review:
Bug 120638: [ATK] Implement new API in AtkText: atk_text_get_string_at_offset()
https://bugs.webkit.org/show_bug.cgi?id=120638

Attachment 210383: Patch proposal plus new Unit tests
https://bugs.webkit.org/attachment.cgi?id=210383&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=210383&action=review


Looks sane to me =)

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1106
> +static gchar* webkitAccessibleTextGetStringAtOffset(AtkText* text, gint
offset, AtkTextGranularity granularity, gint* startOffset, gint* endOffset)

Nit: we have been using gints elsewhere in the a11y implementation, but I'd
rather use the regular types where possible to avoid confusion.

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1110
> +    // ATK_TEXT_BOUNDARY_*_END boundaries, so for now we just need to
translate the

s/END/START/

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1140
> +	   g_assert_not_reached();

ASSERT_NOT_REACHED() instead?

> Source/WebKit/gtk/tests/testatk.c:1402
> +    webkit_web_view_set_settings(webView, settings);

This should not be needed since you're using a settings object you got from the
view instead of creating a new one. Out of curiosity, why do you have to enable
caret browsing here? I thought you were not using the caret to decide where to
get text from.

> Source/WebKit/gtk/tests/testatk.c:1450
> +    webkit_web_view_set_settings(webView, settings);

Ditto.

> Tools/gtk/jhbuild.modules:234
> -    <branch module="pub/GNOME/sources/atk/2.8/atk-2.8.0.tar.xz"
version="2.8.0"
> +    <branch module="pub/GNOME/sources/atk/2.9/atk-2.9.4.tar.xz"
version="2.9.4"
>	       repo="ftp.gnome.org"
> -	      
hash="sha256:b22519176226f3e07cf6d932b77852e6b6be4780977770704b32d0f4e0686df4"/
>
> +	      
hash="sha256:755c9582ca7cf01e5eaa0ac972376877eb365902f388262c21ea3425e0c4d631"/
>

This will help with testing I assume? Do we not need new versions of the at-spi
stuff as well?