[Webkit-unassigned] [Bug 120638] [ATK] Implement new API in AtkText: atk_text_get_string_at_offset()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=120638





--- Comment #4 from Mario Sanchez Prada <mario at webkit.org>  2013-10-09 05:41:24 PST ---
(From update of attachment 210383)
View in context: https://bugs.webkit.org/attachment.cgi?id=210383&action=review

Thanks for the review! See some comments below...

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

This the implementation of a function from AtkText's public API, which explicitly uses gint in the signature so, while I agree with your comment, I guess in this case it's better to leave that one.

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

Good catch. Thanks.

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

Probably yes.

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

You are right about the webkit_web_view_set_settings() part.

About the caret browsing, that's a very good point. It's just here in this patch just because these "new" tests are basically new versions of the ones that are already present, which probably don't need this mode to be enabled anyway other than for testWebkitAtkCaretOffsetsAndExtranousWhiteSpaces. In other words, it might very well a nasty side effect of copy&past that passed overlooked until now. I will remove this and test it locally before landing.

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

Yep

>> Tools/gtk/jhbuild.modules:234
>> +            hash="sha256:755c9582ca7cf01e5eaa0ac972376877eb365902f388262c21ea3425e0c4d631"/>
> 
> This will help with testing I assume? Do we not need new versions of the at-spi stuff as well?

It's really not needed strictly speaking, since the tests here use only ATK and not AT-SPI.

However, raising to a version of AT-SPI that do support the new API (2.9.90, featuring atspi_text_get_string_at_offset) might be a good idea anyway for two reasons:

  1. It keeps the versions of ATK and AT-SPI closer, which is usually a good idea since they use to be released (almost) together
  2. It paves the way for when we move to testing all these things from WebKit2 directly, since we do not have other way to do it there than using at-spi

I will add those bits in the patch before landing

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.