Bug 114870

Summary: [GTK] Reimplement atk_text_get_text_*_offset for CHAR boundary
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, eflews.bot, gyuyoung.kim, jdiggs, mrobinson
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 114867    
Attachments:
Description Flags
Patch proposal
none
Patch proposal
eflews.bot: commit-queue-
Patch proposal
none
Patch proposal mrobinson: review+

Description Mario Sanchez Prada 2013-04-19 06:48:18 PDT
We need to get rid of Pango/Gail dependency, and this is one of the functionalities we would need to fix.
Comment 1 Mario Sanchez Prada 2013-05-09 02:13:09 PDT
Created attachment 201174 [details]
Patch proposal

Patch proposal for this use case. Additional tests are not needed since we are replacing and old implementation, and so this functionality is tested by the testWebkitAtkGetTextAtOffset*() family of functions inside testatk.c
Comment 2 Mario Sanchez Prada 2013-05-09 02:15:33 PDT
Created attachment 201175 [details]
Patch proposal

This is the right one. Seriously.
Comment 3 EFL EWS Bot 2013-05-09 02:18:39 PDT
Comment on attachment 201175 [details]
Patch proposal

Attachment 201175 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/302114
Comment 4 EFL EWS Bot 2013-05-09 02:20:00 PDT
Comment on attachment 201175 [details]
Patch proposal

Attachment 201175 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/416575
Comment 5 Mario Sanchez Prada 2013-05-21 06:12:31 PDT
Created attachment 202422 [details]
Patch proposal

This new patch avoids defining that function for the EFL platform
Comment 6 Martin Robinson 2013-05-21 06:17:45 PDT
Not sure I understand why this is still behind an #ifdef is the code can be shared now. Here's the compilation problem on EFL, I think:

[ 13%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/accessibility/atk/WebKitAccessibleInterfaceText.cpp.o
/mnt/eflews/git/webkit/Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:609:15: warning: unused parameter 'textPosition' [-Wunused-parameter]
/mnt/eflews/git/webkit/Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:578:15: error: 'gchar* webkitAccessibleTextGetChar(AtkText*, gint, GetTextRelativePosition, gint*, gint*)' defined but not used [-Werror=unused-function]
cc1plus: all warnings being treated as errors
make[2]: *** [Source/WebCore/CMakeFiles/webcore_efl.dir/accessibility/atk/WebKitAccessibleInterfaceText.cpp.o] Error 1
make[1]: *** [Source/WebCore/CMakeFiles/webcore_efl.dir/all] Error 2
Comment 7 Mario Sanchez Prada 2013-05-21 06:23:59 PDT
(In reply to comment #6)
> Not sure I understand why this is still behind an #ifdef is the code can be shared now.

I just kept them there not to break EFL behaviour (even if so far it's just "being not implemented"), but I'm currently talking to EFL guys to see what it's best. Probably sharing it, as you mentioned.

In any case, another open question is whether we should move WebCore/accessibility to WebCore/platform/accessibility since, after all, a11y layer is pretty much a platform dependent thing.

Here's the compilation problem on EFL, I think:
> 
> [ 13%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/accessibility/atk/WebKitAccessibleInterfaceText.cpp.o
> /mnt/eflews/git/webkit/Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:609:15: warning: unused parameter 'textPosition' [-Wunused-parameter]
> 

Argh! I should rebase my local branch it seems, to pick Edu's patch.

/mnt/eflews/git/webkit/Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:578:15: error: 'gchar* webkitAccessibleTextGetChar(AtkText*, gint, GetTextRelativePosition, gint*, gint*)' defined but not used [-Werror=unused-function]
> cc1plus: all warnings being treated as errors
> make[2]: *** [Source/WebCore/CMakeFiles/webcore_efl.dir/accessibility/atk/WebKitAccessibleInterfaceText.cpp.o] Error 1
> make[1]: *** [Source/WebCore/CMakeFiles/webcore_efl.dir/all] Error 2

That's what this last patch should have fixed.
Comment 8 Mario Sanchez Prada 2013-05-21 06:50:55 PDT
Created attachment 202426 [details]
Patch proposal

I talked to EFL guys and they do not see any problem in sharing this implementation, now it's free from Pango/Gail stuff, so here it's the patch.

About the other bits (moving stuff into WebCore/platform), I'll send an email about that to discuss it properly.
Comment 9 Martin Robinson 2013-05-21 07:52:12 PDT
Comment on attachment 202426 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=202426&action=review

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:604
> +    if (isEndOfLine(visPos))

Please use visiblePosition here for the variable name.
Comment 10 Mario Sanchez Prada 2013-05-22 09:21:49 PDT
Committed r150518: <http://trac.webkit.org/changeset/150518>