Bug 99011 – [EFL][GTK]Sharing accessibility support in WebKitTestRunner

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mateusz Leszko]

Extending WebKitTestRunner by adding classes needed to support WAI-ARIA.

[Mateusz Leszko]

WebKit-EFL implementation seems to be similar to WebKit-GTK. To eliminate code duplication I propose to share those in one directory, like
./Tools/WebKitTestRunner/InjectionBundle/a11y
Core implementation should be common and any differences should be dealed with platform specific macros.

I propose to move implementation from 
./Tools/WebKitTestRunner/InjectionBundle/gtk/Accessiblity*
to
./Tools/WebKitTestRunner/InjectionBundle/a11y/Accessiblity*

[Mateusz Leszko]

Due to depend bug 99578 I think "atk" folder name would be better. The question is what with other files from folders gtk and efl:
ActivateFontsGtk.cpp
InjectedBundleGtk.cpp
TestRunnerGtk.cpp

We can left it where it is or create:
atk/efl
atk/gtk
folders. Which is better?

[Mateusz Leszko]

Created attachment 171683 [details]
patch

[Mario Sanchez Prada]

(In reply to comment #2)
> Due to depend bug 99578 I think "atk" folder name would be better. The question is what with other files from folders gtk and efl:
> ActivateFontsGtk.cpp
> InjectedBundleGtk.cpp
> TestRunnerGtk.cpp
> 
> We can left it where it is or create:
> atk/efl
> atk/gtk
> folders. Which is better?

Those files are more general than AccessibilityUIElement and AccessibilityController (they are not related to accessibility) and so I don't think putting them under and atk/ path would make sense.

So, I think the approach you're following in the current patch is the right one.

[Mateusz Leszko]

I've compared LayoutTest results, and before and after patch results are the same.

[Grzegorz Czajkowski]

Comment on attachment 171683 [details]
patch

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

> Tools/ChangeLog:3
> +        [EFL] Adding accessibility support for WebKitTestRunner

Generally, this title should be the same as bugzilla's one.

> Tools/ChangeLog:11
> +        (WTR):

If there are no changes in below methods I think we can omit them. The implementation has been just moved. Please leave only those ChangeLogs entries:
* WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp: Renamed from Tools/WebKitTestRunner/InjectedBundle/gtk/AccessibilityControllerGtk.cpp.
* WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp: Renamed from Tools/WebKitTestRunner/InjectedBundle/gtk/AccessibilityUIElementGtk.cpp.

[Mario Sanchez Prada]

Comment on attachment 171683 [details]
patch

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

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp:111
> +    m_stateChangeListenerId = atk_add_global_event_listener(axObjectEventListener, "Gtk:AtkObject:state-change");
> +    m_focusEventListenerId = atk_add_global_event_listener(axObjectEventListener, "Gtk:AtkObject:focus-event");
> +    m_activeDescendantChangedListenerId = atk_add_global_event_listener(axObjectEventListener, "Gtk:AtkObject:active-descendant-changed");
> +    m_childrenChangedListenerId = atk_add_global_event_listener(axObjectEventListener, "Gtk:AtkObject:children-changed");
> +    m_propertyChangedListenerId = atk_add_global_event_listener(axObjectEventListener, "Gtk:AtkObject:property-change");
> +    m_visibleDataChangedListenerId = atk_add_global_event_listener(axObjectEventListener, "Gtk:AtkObject:visible-data-changed");

I think we should stop using the "Gtk" namespace in this lines and replace it with "Atk" or "ATK", which is what it's recommended in the ATK documentation (see [1]). We probably have already avoided using "Gtk" in the first place (I think we did it following the example of other implementations, IIRC).

This change would not impact functionality and would be more coherent with the intention of writing a Gtk independent implementation of AccessibilityUIElement.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:34
> +#include <gtk/gtk.h>

We probably could/should get rid of this include here. I guess.


[1] http://developer.gnome.org/atk/stable/AtkUtil.html#atk-add-global-event-listener

[Grzegorz Czajkowski]

Comment on attachment 171683 [details]
patch

Clearing the review flag regarding to the comments (comment 6, comment 7).

[Mateusz Leszko]

Created attachment 174636 [details]
Accessibility files from gtk folder are moved to atk folder due to common implementation. Event Type naming changed to default, from Gtk to ATK.

[Mateusz Leszko]

(In reply to comment #6)
> (From update of attachment 171683 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171683&action=review
> 
> > Tools/ChangeLog:3
> > +        [EFL] Adding accessibility support for WebKitTestRunner
> 
> Generally, this title should be the same as bugzilla's one.
fixed
> 
> > Tools/ChangeLog:11
> > +        (WTR):
> 
> If there are no changes in below methods I think we can omit them. The implementation has been just moved. Please leave only those ChangeLogs entries:
> * WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp: Renamed from Tools/WebKitTestRunner/InjectedBundle/gtk/AccessibilityControllerGtk.cpp.
> * WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp: Renamed from Tools/WebKitTestRunner/InjectedBundle/gtk/AccessibilityUIElementGtk.cpp.
fixed

[Mateusz Leszko]

(In reply to comment #7)
> (From update of attachment 171683 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171683&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp:111
> > +    m_stateChangeListenerId = atk_add_global_event_listener(axObjectEventListener, "Gtk:AtkObject:state-change");
> > +    m_focusEventListenerId = atk_add_global_event_listener(axObjectEventListener, "Gtk:AtkObject:focus-event");
> > +    m_activeDescendantChangedListenerId = atk_add_global_event_listener(axObjectEventListener, "Gtk:AtkObject:active-descendant-changed");
> > +    m_childrenChangedListenerId = atk_add_global_event_listener(axObjectEventListener, "Gtk:AtkObject:children-changed");
> > +    m_propertyChangedListenerId = atk_add_global_event_listener(axObjectEventListener, "Gtk:AtkObject:property-change");
> > +    m_visibleDataChangedListenerId = atk_add_global_event_listener(axObjectEventListener, "Gtk:AtkObject:visible-data-changed");
> 
> I think we should stop using the "Gtk" namespace in this lines and replace it with "Atk" or "ATK", which is what it's recommended in the ATK documentation (see [1]). We probably have already avoided using "Gtk" in the first place (I think we did it following the example of other implementations, IIRC).
> 
> This change would not impact functionality and would be more coherent with the intention of writing a Gtk independent implementation of AccessibilityUIElement.
fixed
> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:34
> > +#include <gtk/gtk.h>
> 
> We probably could/should get rid of this include here. I guess.
not "build-breaking" after remove so I removed it, as you suggested.

[Mario Sanchez Prada]

Comment on attachment 174636 [details]
Accessibility files from gtk folder are moved to atk folder due to common implementation. Event Type naming changed to default, from Gtk to ATK.

It looks good to me

[Grzegorz Czajkowski]

Comment on attachment 174636 [details]
Accessibility files from gtk folder are moved to atk folder due to common implementation. Event Type naming changed to default, from Gtk to ATK.

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

> Tools/WebKitTestRunner/GNUmakefile.am:97
> +        Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp \

Please fix spaces before landing.

[Build Bot]

Comment on attachment 174636 [details]
Accessibility files from gtk folder are moved to atk folder due to common implementation. Event Type naming changed to default, from Gtk to ATK.

Attachment 174636 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14868407

New failing tests:
inspector-protocol/nmi-webaudio.html

[Mateusz Leszko]

(In reply to comment #14)
> (From update of attachment 174636 [details])
> Attachment 174636 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/14868407
> 
> New failing tests:
> inspector-protocol/nmi-webaudio.html
This test is not connected with the patch.

[Grzegorz Czajkowski]

CC'ing reviewers.

[Mateusz Leszko]

Created attachment 175173 [details]
 Accessibility files from gtk folder are moved to atk folder due to common implementation. Event Type naming changed to default, from Gtk to ATK.

[Mateusz Leszko]

Created attachment 175174 [details]
Accessibility files from gtk folder are moved to atk folder due to common implementation. Event Type naming changed to default, from Gtk to ATK.

[WebKit Review Bot]

Comment on attachment 175174 [details]
Accessibility files from gtk folder are moved to atk folder due to common implementation. Event Type naming changed to default, from Gtk to ATK.

Clearing flags on attachment: 175174

Committed r135267: <http://trac.webkit.org/changeset/135267>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.