Bug 114870 - [GTK] Reimplement atk_text_get_text_*_offset for CHAR boundary
Summary: [GTK] Reimplement atk_text_get_text_*_offset for CHAR boundary
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 114867
  Show dependency treegraph
 
Reported: 2013-04-19 06:48 PDT by Mario Sanchez Prada
Modified: 2013-05-22 09:21 PDT (History)
9 users (show)

See Also:


Attachments
Patch proposal (5.81 KB, patch)
2013-05-09 02:13 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (4.28 KB, patch)
2013-05-09 02:15 PDT, Mario Sanchez Prada
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Patch proposal (5.11 KB, patch)
2013-05-21 06:12 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (4.50 KB, patch)
2013-05-21 06:50 PDT, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>