[GTK] Expose accessibility hierarchy in WebKit2 to ATK/AT-SPI based ATs

[Mario Sanchez Prada]

Currently, the accessibility hierarchy is not being exposed to ATK/AT-SPI based Assistive Technologies (e.g. Orca, Accerciser) in WebKit2, which are just seeing the accessible elements in the UI process at the moment.

Thus, we need to enable WebKit2GTK+ to expose the a11y hierarchy in the Web process through the UI process as it already happens in WebKit1.

[Mario Sanchez Prada]

Created attachment 117752 [details]
Patch proposal

So here it comes the first temptative patch, including:

  - The code needed to plug the a11y hierarchies in the UI and Web processes as if it was just one (from the POV of ATs like Orca)
  - New accessibility unit test for WK2, with all the needed infrastructure to test this thing (mainly a helper process running a simple WebKitView + the actual test inspecting that helper process through ATSPI)
  - New dependency added (required for the unit test): ATSPI2
  - Some changes in the run-gtk-test script, so it makes sure that the accessibility support is available before running the tests

Just one little last thing: in order to be able to get this new unit test running properly, you need to make sure that you have installed both at-spi2-core and the at-spi2 ATK bridge (called at-spi2-atk in some systems, and libatk-adaptor in others). If you have that, and were able to build the patch (which means you have libatspi2.0-dev installed too), run-gtk-test should be able to do all the rest of the magic.

Of course, this initial patch doesn't intend to be 100% feature complete, bug-free or the like... but an initial -yet good-enough- first step to iterate from, in order to provide proper accessibility support in WebKit2GTK.

That's it... for now

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Gustavo Noronha (kov)]

Comment on attachment 117752 [details]
Patch proposal

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

It's looking good, just a first pass.

> Source/WebKit2/ChangeLog:11
> +        Implement the part in the UIProcess, by making the WebView widget

I think you can remove 'Implement the part in the UIProcess' pattern and have just the explanation.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:136
> +    if (priv->accessible) {
> +        g_object_unref(priv->accessible);
> +        priv->accessible = 0;
> +    }
> +

Is there a reason why we don't use GRefPtr for these?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:357
> +    // If the socket as already been created and embedded a plug ID, return it.

s/as/has/

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:378
> +    gchar* plugIDString = g_strdup(plugID.utf8().data());

GOwnPtr?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:68
> +        /* If the widget is no longer alive, save some remote calls
> +           (because of AtkSocket's implementation of ref_state_set())
> +           and just return that this AtkObject is defunct. */

Comments should use C++ style, with // at the start of each line.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:55
> +struct _WebKitWebViewBaseAccessible {
> +    AtkSocket parent;
> +    /*< private >*/
> +    WebKitWebViewBaseAccessiblePrivate* priv;
> +};

No need for padding?

> Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:38
> +    // Sent a message to the parent process telling we're ready.

s/Sent/Send/

[Mario Sanchez Prada]

Created attachment 117799 [details]
Patch proposal

Attaching a new patch addressing the issues pointed out by Gustavo, as well as some improvements/fixes I found useful during this time...

(In reply to comment #3)
> [...]
> > Source/WebKit2/ChangeLog:11
> > +        Implement the part in the UIProcess, by making the WebView widget
> 
> I think you can remove 'Implement the part in the UIProcess' pattern and have just the explanation.

Done.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:136
> > +    if (priv->accessible) {
> > +        g_object_unref(priv->accessible);
> > +        priv->accessible = 0;
> > +    }
> > +
> 
> Is there a reason why we don't use GRefPtr for these?

There's one reason, but it's not a good one: I just forgot.

Fixed now.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:357
> > +    // If the socket as already been created and embedded a plug ID, return it.
> 
> s/as/has/

Fixed

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:378
> > +    gchar* plugIDString = g_strdup(plugID.utf8().data());
> 
> GOwnPtr?

You're right. Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:68
> > +        /* If the widget is no longer alive, save some remote calls
> > +           (because of AtkSocket's implementation of ref_state_set())
> > +           and just return that this AtkObject is defunct. */
> 
> Comments should use C++ style, with // at the start of each line.

Oops! Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:55
> > +struct _WebKitWebViewBaseAccessible {
> > +    AtkSocket parent;
> > +    /*< private >*/
> > +    WebKitWebViewBaseAccessiblePrivate* priv;
> > +};
> 
> No need for padding?

This WebKitWebViewBaseAccessible.h file is not meant to be included in the webkit2gtk headers, as it's gonna be only used internally inside WebKit. In other words, from the outside (e.g. epiphany, the Atk bridge...) you will never see instances of WebKitWebViewBaseAccessible, but instances of AtkObject. So, I don't think there's need for adding any extra padding in this case.

> > Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:38
> > +    // Sent a message to the parent process telling we're ready.
> 
> s/Sent/Send/

Done.

[Gustavo Noronha (kov)]

Comment on attachment 117799 [details]
Patch proposal

Attachment 117799 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10731519

[Collabora GTK+ EWS bot]

Comment on attachment 117799 [details]
Patch proposal

Attachment 117799 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10729547

[Carlos Garcia Campos]

Comment on attachment 117799 [details]
Patch proposal

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:152
> +    priv->accessible = 0;

GRefPtr already initialize the pointer to NULL, so you don't need this here. We should change WebKitWebViewBase to use glib to allocate the private struct so that it will be 0 initialized when allocated.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:374
> +    GOwnPtr<gchar> plugIDString(g_strdup(plugID.utf8().data()));
> +    atk_socket_embed(ATK_SOCKET(priv->accessible.get()), plugIDString.get());

Instead of duplicating the string you can just:

atk_socket_embed(ATK_SOCKET(priv->accessible.get()), plugID.utf8().data());

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:87
> +        bool childIsObject = child == atkObject ? true : false;

bool childIsObject = child == atkObject; that should be enough, I guess.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:98
> +    accessible->priv = new WebKitWebViewBaseAccessiblePrivate();

Use the placement new syntax, see http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:99
> +    accessible->priv->widget = 0;

This won't be needed when allocating the struct with glib and using placement new syntax.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:28
> +#if !defined(__WEBKIT2_H_INSIDE__) && !defined(WEBKIT2_COMPILATION)
> +#error "Only <webkit2/webkit2.h> can be included directly."
> +#endif

Is this header public? if it's private you don't need this here, if it's public you should include it in webkit2.h

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:54
> +    WebKitWebViewBaseAccessiblePrivate* priv;

If the header is public you should use the GNOME coding style, no the WebKit one, see http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:61
> +GType webkit_web_view_base_accessible_get_type();

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:63
> +WebKitWebViewBaseAccessible* webkit_web_view_base_accessible_new(GtkWidget*);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:25
> +#define APP_NAME "AccessibilityTestServer"

Use static variable for this too. What is this for? it doesn't seem to be used.

> Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:27
> +static const char* contents =

We use the k prefix for global static variables, something like kContents, in this case.

> Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:36
> +static gboolean load_finished_cb(WebKitWebLoaderClient *loaderClient, WebKitWebView* webView, gpointer data)

This should be something like loadFinsihedCallback(). The * is incorrectly placed for the first parameter (loaderClient), and parameter names should be omitted because they are not used.

> Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:44
> +main(int argc, char**argv)

there's a space missing between * and argv

> Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:52
> +    webkit_web_view_load_html(webView, contents, 0);

contents is only used here, so it should probably be a local static variable instead of global.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:39
> +#define TEST_SERVER_APP_NAME "AccessibilityTestServer"

Use static const variable here

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:42
> +#define MAX_WAIT_FOR_CHILD 5

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:50
> +    GOwnPtr<char> test_server_path(g_strdup_printf("%s/Programs/WebKit2APITests/" TEST_SERVER_APP_NAME, g_get_current_dir()));

Use webkit coding style for variable names, testServerPath in this case. Instead of g_strdup_printf() it would be better to use g_build_filename(). Use WEBKIT_EXEC_PATH instead of current_dir.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:51
> +    char** test_server_argv = g_strsplit(test_server_path.get(), test_server_path.get(), 1);

Since there aren't parameters, you can use a stack allocated argv of size 2;

char* argv[2];
argv[0] = testServerPath.get();
argv[1] = 0;

This way you fix test_server_argv leak too.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:56
> +    if (!g_spawn_async_with_pipes(0, test_server_argv, 0, (GSpawnFlags)0, 0, 0,

Use a C++ cast.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:64
> +    // Change the child's stdout file descriptor to be non-blocking.
> +    if (fcntl(child_stdout, F_SETFL, fcntl(child_stdout, F_GETFL) | O_NONBLOCK) == -1) {
> +        perror("Error calling fcntl over child's stdout file descriptor");
> +        return FALSE;
> +    }

You could use ia GIOChannel for this:

outChannel = g_io_channel_unix_new(childStdout);
GOwnPtr<GError> error;
g_io_channel_set_flags(outChannel, G_IO_FLAG_NONBLOCK, &error.outPtr());
if (error) {
    g_printerr("Error bla bla bla: %s\n", error->message);
    return FALSE;
}

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:66
> +    // Wait for child to say it's ready till MAX_WAIT_FOR_CHILD.

Using a io channel you can just add a watch for this:

g_io_add_watch(outChannel, G_IO_IN, readStdoutFunction, 0);

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:85
> +    g_return_if_fail(childProcessPid > 0);

Use an asssert instead of g_return

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:88
> +    kill((pid_t)childProcessPid, SIGTERM);

Use a C++ style cast here. Are you sure you need a cast?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:99
> +static void
> +check_atspi_accessible(AtspiAccessible* accessible, const char* targetName, AtspiRole targetRole)

This should be a single line

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:109
> +static GRefPtr<AtspiAccessible>
> +find_test_server_application()

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:115
> +    GRefPtr<AtspiAccessible> current = 0;

GRefPtr already intializes the pointer to NULL on construction.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:118
> +    int i = 0;
> +    for (i = 0; i < childCount; i++) {

you can just use for (int i = 0;

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:196
> +

Would it be possible to start the server here, and stop in afterAll()?

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.h:60
> +WebPageAccessibilityObject* web_page_accessibility_object_new(WebKit::WebPage*);
> +
> +void web_page_accessibility_object_refresh(WebPageAccessibilityObject*);

Use WebKit coding style for these, since they are not generated and this is not public API.

> Tools/Scripts/run-gtk-tests:28
> +                "WebKit2APITests/AccessibilityTestServer" ]

Why do you need this? only executables starting with Test are considered unit tests.

> Tools/Scripts/run-gtk-tests:96
> +    paths_to_check = [ "/usr/libexec",
> +                       "/usr/lib/at-spi2-core",
> +                       "/usr/lib32/at-spi2-core"
> +                       "/usr/lib64/at-spi2-core" ]

Isn't there a better way? this wouldn't work if the at-spli2-code executable is in another prefix

> Tools/Scripts/run-gtk-tests:98
> +        filepath = "%s/%s" % (path, filename)

Use os.path.join(path, filename)

> configure.ac:1136
> +# Require atspi2 only when building Webkit2 (required for unit tests).
> +if test "$enable_webkit2" = "yes"; then
> +   PKG_CHECK_MODULES([ATSPI2],
> +                     [atspi-2 >= $ATSPI2_REQUIRED_VERSION])
> +   AC_SUBST([ATSPI2_CFLAGS])
> +   AC_SUBST([ATSPI2_LIBS])
> +fi

If it's only required for unit tests, this should be an optional dependency, so that the test that depend on this is only built when atspi2 is present.

[Mario Sanchez Prada]

Created attachment 120955 [details]
Patch proposal

First of all, thanks for your detailed review. Now attaching a new patch where I think I've addressed all the issues you pointed out, with some exceptions where I thought it was not possible.

See my comments below....

(In reply to comment #7)
> (From update of attachment 117799 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117799&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:152
> > +    priv->accessible = 0;
> 
> GRefPtr already initialize the pointer to NULL, so you don't need this here. We should change WebKitWebViewBase to use glib to allocate the private struct so that it will be 0 initialized when allocated.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:374
> > +    GOwnPtr<gchar> plugIDString(g_strdup(plugID.utf8().data()));
> > +    atk_socket_embed(ATK_SOCKET(priv->accessible.get()), plugIDString.get());
> 
> Instead of duplicating the string you can just:
> 
> atk_socket_embed(ATK_SOCKET(priv->accessible.get()), plugID.utf8().data());

Fixed.

JFTR, I needed to const_cast plugID.utf8().data() for that to work, since atk_socket_embed()'s signature expects a gchar* in there, even if it treats it as a const gchar* at the end (weird, but it's like that).

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:87
> > +        bool childIsObject = child == atkObject ? true : false;
> 
> bool childIsObject = child == atkObject; that should be enough, I guess.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:98
> > +    accessible->priv = new WebKitWebViewBaseAccessiblePrivate();
> 
> Use the placement new syntax, see http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:99
> > +    accessible->priv->widget = 0;
> 
> This won't be needed when allocating the struct with glib and using placement new syntax.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:28
> > +#if !defined(__WEBKIT2_H_INSIDE__) && !defined(WEBKIT2_COMPILATION)
> > +#error "Only <webkit2/webkit2.h> can be included directly."
> > +#endif
> 
> Is this header public? if it's private you don't need this here, if it's public you should include it in webkit2.h

The header is not public, so you're right: I don't need that there. Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:54
> > +    WebKitWebViewBaseAccessiblePrivate* priv;
> 
> If the header is public you should use the GNOME coding style, no the WebKit one, see http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

It's not public, so kept using WebKit's coding style here.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:61
> > +GType webkit_web_view_base_accessible_get_type();
> 
> Ditto.

I can't change this one to WK's coding style, since its implementation comes from expanding the G_DEFINE macro.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:63
> > +WebKitWebViewBaseAccessible* webkit_web_view_base_accessible_new(GtkWidget*);
> 
> Ditto.

I can change this one, though. Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:25
> > +#define APP_NAME "AccessibilityTestServer"
> 
> Use static variable for this too. What is this for? it doesn't seem to be used.

Argh! Legacy stuff from a previous version of the patch. Removed.

> > Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:27
> > +static const char* contents =
> 
> We use the k prefix for global static variables, something like kContents, in this case.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:36
> > +static gboolean load_finished_cb(WebKitWebLoaderClient *loaderClient, WebKitWebView* webView, gpointer data)
> 
> This should be something like loadFinsihedCallback(). The * is incorrectly placed for the first parameter (loaderClient), and parameter names should be omitted because they are not used.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:44
> > +main(int argc, char**argv)
> 
> there's a space missing between * and argv

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:52
> > +    webkit_web_view_load_html(webView, contents, 0);
> 
> contents is only used here, so it should probably be a local static variable instead of global.

Fixed (removed the variable, just used the literal string in the only place it's used).

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:39
> > +#define TEST_SERVER_APP_NAME "AccessibilityTestServer"
> 
> Use static const variable here

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:42
> > +#define MAX_WAIT_FOR_CHILD 5
> 
> Ditto.

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:50
> > +    GOwnPtr<char> test_server_path(g_strdup_printf("%s/Programs/WebKit2APITests/" TEST_SERVER_APP_NAME, g_get_current_dir()));
> 
> Use webkit coding style for variable names, testServerPath in this case. Instead of g_strdup_printf() it would be better to use g_build_filename(). Use WEBKIT_EXEC_PATH instead of current_dir.

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:51
> > +    char** test_server_argv = g_strsplit(test_server_path.get(), test_server_path.get(), 1);
> 
> Since there aren't parameters, you can use a stack allocated argv of size 2;
> 
> char* argv[2];
> argv[0] = testServerPath.get();
> argv[1] = 0;
> 
> This way you fix test_server_argv leak too.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:56
> > +    if (!g_spawn_async_with_pipes(0, test_server_argv, 0, (GSpawnFlags)0, 0, 0,
> 
> Use a C++ cast.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:64
> > +    // Change the child's stdout file descriptor to be non-blocking.
> > +    if (fcntl(child_stdout, F_SETFL, fcntl(child_stdout, F_GETFL) | O_NONBLOCK) == -1) {
> > +        perror("Error calling fcntl over child's stdout file descriptor");
> > +        return FALSE;
> > +    }
> 
> You could use ia GIOChannel for this:
> 
> outChannel = g_io_channel_unix_new(childStdout);
> GOwnPtr<GError> error;
> g_io_channel_set_flags(outChannel, G_IO_FLAG_NONBLOCK, &error.outPtr());
> if (error) {
>     g_printerr("Error bla bla bla: %s\n", error->message);
>     return FALSE;
> }
> 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:66
> > +    // Wait for child to say it's ready till MAX_WAIT_FOR_CHILD.
> 
> Using a io channel you can just add a watch for this:
> 
> g_io_add_watch(outChannel, G_IO_IN, readStdoutFunction, 0);

Already thought of that, but I can't use a GIO channel here because it uses sources and the main loop to handle events through that callback, and I can't afford that because it would mean that the execution of the test function's code will continue after adding the watch, therefore finishing the test before having a chance to handle the input data coming through the channel.

Instead, I need to *block* on that point and wait either until some data has been received from the child process or a timeout has ocurred, in order to know that I'm ready to start testing things.

So, I haven't changed anything in this regard. It still looks to me like the current approach is the one we need in here.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:85
> > +    g_return_if_fail(childProcessPid > 0);
> 
> Use an asssert instead of g_return

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:88
> > +    kill((pid_t)childProcessPid, SIGTERM);
> 
> Use a C++ style cast here. Are you sure you need a cast?

No, I don't need it. You're right. Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:99
> > +static void
> > +check_atspi_accessible(AtspiAccessible* accessible, const char* targetName, AtspiRole targetRole)
> 
> This should be a single line

Fixed. Also, changed the name to match WK's coding style.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:109
> > +static GRefPtr<AtspiAccessible>
> > +find_test_server_application()
> 
> Ditto.

Fixed. Also, changed the name to match WK's coding style.
 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:115
> > +    GRefPtr<AtspiAccessible> current = 0;
> 
> GRefPtr already intializes the pointer to NULL on construction.

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:118
> > +    int i = 0;
> > +    for (i = 0; i < childCount; i++) {
> 
> you can just use for (int i = 0;

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:196
> > +
> 
> Would it be possible to start the server here, and stop in afterAll()?

Good point. I think it should be possible and, actually, I've tried the change and seems to work fine, so I included it in the new patch.

The only catch here is in the case that we want to run different tests (that is, servers loading different HTML's) for each unit test, as this approach wouldn't work, but at the moment that doesn't sound like and issue :-)

So, fixed.

> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.h:60
> > +WebPageAccessibilityObject* web_page_accessibility_object_new(WebKit::WebPage*);
> > +
> > +void web_page_accessibility_object_refresh(WebPageAccessibilityObject*);
> 
> Use WebKit coding style for these, since they are not generated and this is not public API.

Fixed.

> > Tools/Scripts/run-gtk-tests:28
> > +                "WebKit2APITests/AccessibilityTestServer" ]
> 
> Why do you need this? only executables starting with Test are considered unit tests.

I needed it because run-gtk-tests doesn't ignore executable files not starting with "test" or "Test".

However I agree with you it would be better that way, so I filed bug 75474 to fix that and made it block this one. Thus, this new patch assumes run-gtk-tests actually works that way, so Fixed.

> > Tools/Scripts/run-gtk-tests:96
> > +    paths_to_check = [ "/usr/libexec",
> > +                       "/usr/lib/at-spi2-core",
> > +                       "/usr/lib32/at-spi2-core"
> > +                       "/usr/lib64/at-spi2-core" ]
> 
> Isn't there a better way? this wouldn't work if the at-spli2-code executable is in another prefix

Not that I know of.

The problem is that, depending on the distro, the path where the at-spi2-registryd and at-spi-bus-launcher files are is different, and not only that, but also the name of the package itself, so it would be more error-prone and complex, IMHO, to try to automatically find the place where those files are in the system thatn just looking for them in some (well-known) typical places.

I think this approach is already done for some other stuff in WK (fonts, I think), and it seems to make sense to me in this case as well. If one day we try to run this in a machine with those files in a different path (unlikely, but possible), it would be a matter of adding that path to the list :-)

> > Tools/Scripts/run-gtk-tests:98
> > +        filepath = "%s/%s" % (path, filename)
> 
> Use os.path.join(path, filename)

Fixed.

> > configure.ac:1136
> > +# Require atspi2 only when building Webkit2 (required for unit tests).
> > +if test "$enable_webkit2" = "yes"; then
> > +   PKG_CHECK_MODULES([ATSPI2],
> > +                     [atspi-2 >= $ATSPI2_REQUIRED_VERSION])
> > +   AC_SUBST([ATSPI2_CFLAGS])
> > +   AC_SUBST([ATSPI2_LIBS])
> > +fi
> 
> If it's only required for unit tests, this should be an optional dependency, so that the test that depend on this is only built when atspi2 is present.

Very true. Fixed.

[Collabora GTK+ EWS bot]

Comment on attachment 120955 [details]
Patch proposal

Attachment 120955 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11081013

[Mario Sanchez Prada]

(In reply to comment #9)
> (From update of attachment 120955 [details])
> Attachment 120955 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/11081013

This happens because of a bug in the version of atspi installed in the EWS machine, which is not including a wrong DBus header in one of libatspi header files. Fortunately, it has been fixed in more recent versions of libatspi, but will keep failing with no remedy after the EWS machine upgrades the package to it.

[Carlos Garcia Campos]

Comment on attachment 120955 [details]
Patch proposal

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:341
> +static AtkObject* webkitWebViewBaseGetAccessible(GtkWidget *widget)

GtkWidget *widget -> GtkWidget* widget

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:64
> +static AtkStateSet* webkitWebViewBaseAccessibleRefStateSet(AtkObject *atkObject)

AtkObject *atkObject -> AtkObject* atkObject

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:68
> +    AtkStateSet* state_set;

state_set -> stateSet

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:83
> +static gint webkitWebViewBaseAccessibleGetIndexInParent(AtkObject *atkObject)

AtkObject *atkObject -> AtkObject* atkObject

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:85
> +    g_return_val_if_fail(ATK_IS_OBJECT(atkObject), -1);

Don't use g_return macros in private static code, note that atk_object_get_index_in_parent() already has that check, so it doesn't get here if pointer is not a valid AtkObject.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:128
> +    AtkObject* object = static_cast<AtkObject*>(g_object_new(WEBKIT_TYPE_WEB_VIEW_BASE_ACCESSIBLE, 0));

Use ATK_OBJECT() instead of static_cast. Use NULL instead of 0 in g_object_new() since it's a sentinel

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:31
> +#include <webkit2/WebKitDefines.h>

Do you need this?

> Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:55
> +    WebKitWebLoaderClient* loaderClient = webkit_web_view_get_loader_client(webView);

Loader client has been already removed, use WebKitWebView::load-changed instead.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:58
> +    int child_stdout = 0;

child_stdout -> childStdout

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:113
> +    GRefPtr<AtspiAccessible> desktop = atspi_get_desktop(0);

atspi_get_desktop returns a new ref, so you should use adoptGRef() here.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:136
> +    GRefPtr<AtspiAccessible> currentChild = atspi_accessible_get_child_at_index(currentParent.get(), 0, 0);

I think you should use adoptGRef here too. Use Test::assertObjectIsDeletedWhenTestFinishes() to make sure you don't leak anything.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:78
> +    g_return_val_if_fail(ATK_IS_OBJECT(atkObject), -1);

Don't use g_return macro here.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:110
> +    accessible->m_page = 0;

accessible is already initialized to 0 when allocated.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:128
> +    AtkObject* object = static_cast<AtkObject*>(g_object_new(WEB_TYPE_PAGE_ACCESSIBILITY_OBJECT, 0));

Use NULL instead of 0 in g_object_new

> Tools/Scripts/run-gtk-tests:98
> +    paths_to_check = [ "/usr/libexec",
> +                       "/usr/lib/at-spi2-core",
> +                       "/usr/lib32/at-spi2-core"
> +                       "/usr/lib64/at-spi2-core" ]

Maybe we can use pkg-config --variable=exec_prefix atspi-2 ?

[Carlos Garcia Campos]

(In reply to comment #8)
> Created an attachment (id=120955) [details]
> Patch proposal

> > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:64
> > > +    // Change the child's stdout file descriptor to be non-blocking.
> > > +    if (fcntl(child_stdout, F_SETFL, fcntl(child_stdout, F_GETFL) | O_NONBLOCK) == -1) {
> > > +        perror("Error calling fcntl over child's stdout file descriptor");
> > > +        return FALSE;
> > > +    }
> > 
> > You could use ia GIOChannel for this:
> > 
> > outChannel = g_io_channel_unix_new(childStdout);
> > GOwnPtr<GError> error;
> > g_io_channel_set_flags(outChannel, G_IO_FLAG_NONBLOCK, &error.outPtr());
> > if (error) {
> >     g_printerr("Error bla bla bla: %s\n", error->message);
> >     return FALSE;
> > }
> > 
> > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:66
> > > +    // Wait for child to say it's ready till MAX_WAIT_FOR_CHILD.
> > 
> > Using a io channel you can just add a watch for this:
> > 
> > g_io_add_watch(outChannel, G_IO_IN, readStdoutFunction, 0);
> 
> Already thought of that, but I can't use a GIO channel here because it uses sources and the main loop to handle events through that callback, and I can't afford that because it would mean that the execution of the test function's code will continue after adding the watch, therefore finishing the test before having a chance to handle the input data coming through the channel.
> 
> Instead, I need to *block* on that point and wait either until some data has been received from the child process or a timeout has ocurred, in order to know that I'm ready to start testing things.
> 
> So, I haven't changed anything in this regard. It still looks to me like the current approach is the one we need in here.
> 

If you need to block, why don't use g_spawn_sync() instead of the async version?

[Mario Sanchez Prada]

(In reply to comment #12)
> [...]
> If you need to block, why don't use g_spawn_sync() instead of the async version?

Because I need to block in a way that I need to wait for the child to send me an "OK" message when it's ready but, at the same time, I don't want to wait forever in case something goes wrong in the child process (imagine it hangs for some reason), so I need to be able to unblock if a certain amount of time (a timeout) has happened before getting such a response, which is something I can't specify with g_spawn_sync() I think.

So, that's why I used g_spawn_async() instead: to have more control over the whole thing in a way g_spawn_sync() wouldn't allow. Anyway, according to docs, g_spawn_sync() "calls g_spawn_async_with_pipes() internally" :-)

Anyway, if there's a way to handle that timeout without having to do this manually, I'd be glad to know of it and more than happy to change the implementation of this part, of course.

[Carlos Garcia Campos]

(In reply to comment #13)
> (In reply to comment #12)
> > [...]
> > If you need to block, why don't use g_spawn_sync() instead of the async version?
> 
> Because I need to block in a way that I need to wait for the child to send me an "OK" message when it's ready but, at the same time, I don't want to wait forever in case something goes wrong in the child process (imagine it hangs for some reason), so I need to be able to unblock if a certain amount of time (a timeout) has happened before getting such a response, which is something I can't specify with g_spawn_sync() I think.
> 
> So, that's why I used g_spawn_async() instead: to have more control over the whole thing in a way g_spawn_sync() wouldn't allow. Anyway, according to docs, g_spawn_sync() "calls g_spawn_async_with_pipes() internally" :-)
> 
> Anyway, if there's a way to handle that timeout without having to do this manually, I'd be glad to know of it and more than happy to change the implementation of this part, of course.

static bool kChildFinished = true;

G_GNUC_NORETURN static gpointer timeoutMonitor(gpointer)
{
        g_usleep(kMaxWaitForChild * G_USEC_PER_SEC);

        if (kChildFinished)
                g_thread_exit(0);

        g_print("Timeout");
        exit (0);
}

static void timeoutMonitorStart()
{
        kChildFinished = false;
#if (!GLIB_CHECK_VERSION(2,31,0))
        g_thread_create(timeoutMonitor, 0, FALSE, 0);
#else
     	g_thread_new("TimeoutMonitor", timeoutMonitor, 0);
#endif
}

static void timeoutMonitorStop()
{
 	kChildFinished = true;
}

....

timeoutMonitorStart();
g_spawn_sync();
timeoutMonitorStop();

[Mario Sanchez Prada]

Thanks for the suggestion and the code snippet Carlos. However I have a doubt: do you think this solution (using another thread to kill the main one on timeout) is better/cleaner than the current approach?

I honestly have my doubts... looks to me a bit too complex for something that, in the end, is very simple: I want,  _after_ launching the a11y server, to block the current execution until the child process finishes or a timeout has occurred, which is something perfectly doable without using extra threads.

Perhaps I'm missing something... what's your take?

(In reply to comment #14)
> [...]
> static bool kChildFinished = true;
> 
> G_GNUC_NORETURN static gpointer timeoutMonitor(gpointer)
> {
>         g_usleep(kMaxWaitForChild * G_USEC_PER_SEC);
> 
>         if (kChildFinished)
>                 g_thread_exit(0);
> 
>         g_print("Timeout");
>         exit (0);
> }
> 
> static void timeoutMonitorStart()
> {
>         kChildFinished = false;
> #if (!GLIB_CHECK_VERSION(2,31,0))
>         g_thread_create(timeoutMonitor, 0, FALSE, 0);
> #else
>          g_thread_new("TimeoutMonitor", timeoutMonitor, 0);
> #endif
> }
> 
> static void timeoutMonitorStop()
> {
>      kChildFinished = true;
> }
> 
> ....
> 
> timeoutMonitorStart();
> g_spawn_sync();
> timeoutMonitorStop();

[Carlos Garcia Campos]

(In reply to comment #15)
> Thanks for the suggestion and the code snippet Carlos. However I have a doubt: do you think this solution (using another thread to kill the main one on timeout) is better/cleaner than the current approach?

Yes, I think so, and it's less error prone. For example your patch doesn't handle EINTR when reading, I think it should use select, since the descriptor is non-blocking to know when to read, and it's leaking the stdout desriptor that should be closed after reading. All of that (and more) is correctly handled by g_spawn_sync() and you have the output directly in a variable. But I'm fine with your solution (fixing the problems I mentioned) if you think it's better.

[Martin Robinson]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 120955 [details] [details])
> > Attachment 120955 [details] [details] did not pass gtk-ews (gtk):
> > Output: http://queues.webkit.org/results/11081013
> 
> This happens because of a bug in the version of atspi installed in the EWS machine, which is not including a wrong DBus header in one of libatspi header files. Fortunately, it has been fixed in more recent versions of libatspi, but will keep failing with no remedy after the EWS machine upgrades the package to it.

You should probably add this version of atspi to the jhbuild modules.

[Mario Sanchez Prada]

(In reply to comment #16)
> (In reply to comment #15)
> > Thanks for the suggestion and the code snippet Carlos. However I have a doubt: do you think this solution 
> >(using another thread to kill the main one on timeout) is better/cleaner than the current approach?
> 
> Yes, I think so, and it's less error prone. 

Hmm... there's another reason *not* to use g_spawn_sync() that I forgot to mentino before, and it's quite important: I need the AccessibilityTestServer to keep running as a different process while I test all the a11y related stuff from TestWebKitAccessibility, so I can't synchronously spawn that process as you suggest with that snippet of code.

The thing about blocking and having the timeout, that perhaps I explained poorly before, is that I don't want to wait forever in case the AccessibilityTestServer doesn't works as expected (starts a basic app, load some HTML and notify the parent process), so I need a mechanism to kill that child process from TestWebKitAccessibility not to continue with the unit tests.

But, if the a11y server runs and starts fine (so it sends the "OK" message to its parent), I want it to keep running as a separate process while I perform all the a11y-related unit test over it (checking the a11y hierarchy through ATSPI), and will exit only once the parent process exits.

> For example your patch doesn't handle EINTR when reading, I think it should use select, since the descriptor
> is non-blocking to know when to read, and it's leaking the stdout desriptor that should be closed after
> reading. All of that (and more) is correctly handled by g_spawn_sync() and you have the output directly in
> a variable. But I'm fine with your solution (fixing the problems I mentioned) if you think it's better.

Because of the reasons exposed (and sorry for not bringing this last -very important- point up before), I think it's better to keep doing what I did, perhaps adding some extra checks as you suggest, and reviewing the code not to leak anything.

What do you think?

[Mario Sanchez Prada]

(In reply to comment #17)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (From update of attachment 120955 [details] [details] [details])
> > > Attachment 120955 [details] [details] [details] did not pass gtk-ews (gtk):
> > > Output: http://queues.webkit.org/results/11081013
> > 
> > This happens because of a bug in the version of atspi installed in the EWS machine, which is not including a wrong DBus header in one of libatspi header files. Fortunately, it has been fixed in more recent versions of libatspi, but will keep failing with no remedy after the EWS machine upgrades the package to it.
> 
> You should probably add this version of atspi to the jhbuild modules.

In the new patch I'm writing, I added at-spi2-core and at-spi2-atk as jhbuild modules for testing. That way I understand there wouldn't be longer needed to have those packages installed in the bots, as they would use them right from jhbuild.

About the version, I specified at-spi2 2.2.1, which is what I'm using in Fedora now and seems not to present the issue in the version available in the bots (2.2.0).

[Carlos Garcia Campos]

(In reply to comment #18)

> Because of the reasons exposed (and sorry for not bringing this last -very important- point up before), I think it's better to keep doing what I did, perhaps adding some extra checks as you suggest, and reviewing the code not to leak anything.
> 
> What do you think?

I see, you don't actually want to block when calling the process but when reading from stdout, and kill the process after a timeout. In that case, do not set the descriptor as non-blocking, no?

[Mario Sanchez Prada]

(In reply to comment #20)
> (In reply to comment #18)
> 
> > Because of the reasons exposed (and sorry for not bringing this last -very important- point up before), I think it's better to keep doing what I did, perhaps adding some extra checks as you suggest, and reviewing the code not to leak anything.
> > 
> > What do you think?
> 
> I see, you don't actually want to block when calling the process but when reading from stdout, and kill the process after a timeout. In that case, do not set the descriptor as non-blocking, no?

Argh! I explain poorly, definitely :-)

I both want and don't want to block, beacuse of differente reasons, and at different points:

  * I don't wan't to block (so don't want to use g_spawn_sync()) because, if everything goes fine (server running and loading the test HTML properly), I want the server to keep running as a child process until I don't need it anymore. And this could mean keeping it alive and running while executing one or more tests.

  * I want to block exactly after spawning the child process with spawn_async and before exiting the runTestServer() function, because I don't want any test to start running before being sure the server is ready, which is what I know through the "OK" message.

  * I can't use GIO channels and the add_watch() thing, because I can't rely on the main loop to handle the response from the child process, since that would mean exiting runTestServer() too soon (before handling the "OK" message). This is related to the previous point (why I want to block there).

  * I need the non-blocking thing in the file descriptor to implement the timeout thing so, in case something goes wrong in the child process, we make sure to kill it after some time, so the test doesn't hang.

Hope this time I explained better :-)

[Carlos Garcia Campos]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #18)
> > 
> > > Because of the reasons exposed (and sorry for not bringing this last -very important- point up before), I think it's better to keep doing what I did, perhaps adding some extra checks as you suggest, and reviewing the code not to leak anything.
> > > 
> > > What do you think?
> > 
> > I see, you don't actually want to block when calling the process but when reading from stdout, and kill the process after a timeout. In that case, do not set the descriptor as non-blocking, no?
> 
> Argh! I explain poorly, definitely :-)

I understood it :-)

> I both want and don't want to block, beacuse of differente reasons, and at different points:
> 
>   * I don't wan't to block (so don't want to use g_spawn_sync()) because, if everything goes fine (server running and loading the test HTML properly), I want the server to keep running as a child process until I don't need it anymore. And this could mean keeping it alive and running while executing one or more tests.

ok, g_spawn_async_with_pipes() then.

>   * I want to block exactly after spawning the child process with spawn_async and before exiting the runTestServer() function, because I don't want any test to start running before being sure the server is ready, which is what I know through the "OK" message.

That's what I meant in my previous comment, you want to block when reading stdout. Do not set the file descriptor as non-blocking and use read or even g_io_channel_read_chars().

>   * I can't use GIO channels and the add_watch() thing, because I can't rely on the main loop to handle the response from the child process, since that would mean exiting runTestServer() too soon (before handling the "OK" message). This is related to the previous point (why I want to block there).

Yes, that's why I proposed to use a blocking file descriptor, you don't need to use watchers, because read will block.

>   * I need the non-blocking thing in the file descriptor to implement the timeout thing so, in case something goes wrong in the child process, we make sure to kill it after some time, so the test doesn't hang.

For the timeout thing you can use the thread killer I proposed. 

> Hope this time I explained better :-)

Yes :-) If you think it's better to use loop + non-blocking read + sleep, just ignore my proposal and fix your current one.

[Mario Sanchez Prada]

(In reply to comment #22)
> [...]
> Yes :-) If you think it's better to use loop + non-blocking read + sleep, just ignore my proposal and fix your current one.

I finally used your suggestion of "killer thread" + GIOChannel, but with g_spawn_async(). You'll see it in the patch I'm attaching soonish... :-)

Thanks for your (very helpful) feedback

[Mario Sanchez Prada]

Created attachment 121681 [details]
Patch proposal

Thanks again Carlos for your feedback. Now attaching another patch addressing the issues you pointed out, as well as incorporating Martin's suggestion of adding at-spi2-{core|atk} (and the needed version) to jhbuild modules.

As for the g_spawn_sync thing (pointed out in a separate comments), I made sure to close the file descriptor (which was leaking as you noticed) and, finally, changed the code to use your suggestion of the "killer thread" + the GIOChannel... but with g_spawn_async(), as you also pointed out in your last comment (Thanks!)

So, I think we've reached some consensus here, or at least I hope so... :-)

Now, see some comments inline below...

(In reply to comment #11)
> (From update of attachment 120955 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120955&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:341
> > +static AtkObject* webkitWebViewBaseGetAccessible(GtkWidget *widget)
> 
> GtkWidget *widget -> GtkWidget* widget
>
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:64
> > +static AtkStateSet* webkitWebViewBaseAccessibleRefStateSet(AtkObject *atkObject)
> 
> AtkObject *atkObject -> AtkObject* atkObject 
>
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:68
> > +    AtkStateSet* state_set;
> 
> state_set -> stateSet
>
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:83
> > +static gint webkitWebViewBaseAccessibleGetIndexInParent(AtkObject *atkObject)
> 
> AtkObject *atkObject -> AtkObject* atkObject
> 
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:85
> > +    g_return_val_if_fail(ATK_IS_OBJECT(atkObject), -1);
> 
> Don't use g_return macros in private static code, note that atk_object_get_index_in_parent() already has that check, so it doesn't get here if pointer is not a valid AtkObject.
> 
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:128
> > +    AtkObject* object = static_cast<AtkObject*>(g_object_new(WEBKIT_TYPE_WEB_VIEW_BASE_ACCESSIBLE, 0));
> 
> Use ATK_OBJECT() instead of static_cast. Use NULL instead of 0 in g_object_new() since it's a sentinel
> 
Fixed (Done the same fix in WebPageAccessibilityObject, btw).

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:31
> > +#include <webkit2/WebKitDefines.h>
> 
> Do you need this?
> 
Not actually. Removed.

> > Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:55
> > +    WebKitWebLoaderClient* loaderClient = webkit_web_view_get_loader_client(webView);
> 
> Loader client has been already removed, use WebKitWebView::load-changed instead.
> 
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:58
> > +    int child_stdout = 0;
> 
> child_stdout -> childStdout
> 
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:113
> > +    GRefPtr<AtspiAccessible> desktop = atspi_get_desktop(0);
> 
> atspi_get_desktop returns a new ref, so you should use adoptGRef() here.
> 
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:136
> > +    GRefPtr<AtspiAccessible> currentChild = atspi_accessible_get_child_at_index(currentParent.get(), 0, 0);
> 
> I think you should use adoptGRef here too.
>
You are right. Fixed.

> Use Test::assertObjectIsDeletedWhenTestFinishes() to make sure you don't leak anything.
> 
I can't use that, since the test doesn't own the object at all, which has a ref_count greater than 1 both when retrieving it here and when finishing the test, since libatspi keeps the full a11y hierarchy cached and so several references to these objects, which therefore won't have a ref_count of 0 by any means with finishing the test, so they won't get deleted at that point.

Still, your point has been helpful anyway, since it's true I was leaking one reference by no using adoptGRef(). Thanks.

> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:78
> > +    g_return_val_if_fail(ATK_IS_OBJECT(atkObject), -1);
> 
> Don't use g_return macro here.
> 
Fixed.

> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:110
> > +    accessible->m_page = 0;
> 
> accessible is already initialized to 0 when allocated.
> 
Fixed.

> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:128
> > +    AtkObject* object = static_cast<AtkObject*>(g_object_new(WEB_TYPE_PAGE_ACCESSIBILITY_OBJECT, 0));
> 
> Use NULL instead of 0 in g_object_new
> 
Fixed.

> > Tools/Scripts/run-gtk-tests:98
> > +    paths_to_check = [ "/usr/libexec",
> > +                       "/usr/lib/at-spi2-core",
> > +                       "/usr/lib32/at-spi2-core"
> > +                       "/usr/lib64/at-spi2-core" ]
> 
> Maybe we can use pkg-config --variable=exec_prefix atspi-2 ?

Fixed.

[WebKit Review Bot]

Attachment 121681 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/ChangeLog', u..." exit_code: 1

WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h"
Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:53:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:84:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 121681 [details]
Patch proposal

Attachment 121681 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11184581

[Mario Sanchez Prada]

Created attachment 121688 [details]
Patch proposal

New patch avoiding the errors triggered by the last one...

(In reply to comment #25)
> Attachment 121681 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/ChangeLog', u..." exit_code: 1
> 
> WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h"
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:53:  Missing space before ( in if(  [whitespace/parens] [5]
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:84:  Missing space after ,  [whitespace/comma] [3]
> Total errors found: 2 in 22 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Forgot to run check-webkit-style before uploading the patch. Sorry!

(In reply to comment #26)
> (From update of attachment 121681 [details])
> Attachment 121681 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/11184581

ATSPI2 is an optional dependency, and in this case it's failing to build in the EWS because the package is there, but its version is not recent enough. In that case, the configure script should just say that ATSPI2 is not available and continue, instead of stopping there. Fixed in this new patch as well.

[Gustavo Noronha (kov)]

Comment on attachment 121688 [details]
Patch proposal

Attachment 121688 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11166585

[Carlos Garcia Campos]

Comment on attachment 121688 [details]
Patch proposal

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

It looks good to me, great work!. I have just a few more comments, sorry :-P

> Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:33
> +int
> +main(int argc, char** argv)

This should be one line.

> Tools/Scripts/run-gtk-tests:77
> +        for path in paths_to_check:
> +            filepath = os.path.join(exec_prefix, path, filename)
> +            if os.path.isfile(filepath):
> +                return filepath
> +            return None

You are only checking the first path, the return should be after the loop.

> Tools/Scripts/run-gtk-tests:97
> +        Executive().popen(["xprop", "-root", "-remove", "AT_SPI_BUS"],
> +                          env=test_env, stderr=subprocess.PIPE, stdout=subprocess.PIPE)

Don't use pipes if you are not going to read/write from/to them

> Tools/Scripts/run-gtk-tests:105
> +                a11y_bus_launcher = Executive().popen([a11y_bus_launcher_path],
> +                                                      env=test_env, stderr=subprocess.PIPE, stdout=subprocess.PIPE)

Ditto.

> Tools/Scripts/run-gtk-tests:116
> +                a11y_registryd = Executive().popen(["%s/at-spi2-registryd" % atspi2_exec_path],
> +                                                   env=test_env, stderr=subprocess.PIPE, stdout=subprocess.PIPE)

Ditto.

[Mario Sanchez Prada]

Created attachment 121814 [details]
Patch proposal

(In reply to comment #29)
> (From update of attachment 121688 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121688&action=review
> 
> It looks good to me, great work!. I have just a few more comments, sorry :-P

No worries. Everytime you point something out the patch gets better, so I'm more than happy to address those issues.

Now attaching a new patch taking care of these new ones. See my comments below...

> > Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:33
> > +int
> > +main(int argc, char** argv)
> 
> This should be one line.

Fixed.

> > Tools/Scripts/run-gtk-tests:77
> > +        for path in paths_to_check:
> > +            filepath = os.path.join(exec_prefix, path, filename)
> > +            if os.path.isfile(filepath):
> > +                return filepath
> > +            return None
> 
> You are only checking the first path, the return should be after the loop.

Argh! Dammed python indentation thing... Fixed.
 
> > Tools/Scripts/run-gtk-tests:97
> > +        Executive().popen(["xprop", "-root", "-remove", "AT_SPI_BUS"],
> > +                          env=test_env, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
> 
> Don't use pipes if you are not going to read/write from/to them

Fixed. I also switched to run_command() here since for 'xprop' I won't need the process later on to kill it (it's not a daemon).

> > Tools/Scripts/run-gtk-tests:105
> > +                a11y_bus_launcher = Executive().popen([a11y_bus_launcher_path],
> > +                                                      env=test_env, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
> 
> Ditto.

Fixed. In this case I kept using popen() because I need to make sure I kill the process later on.

> > Tools/Scripts/run-gtk-tests:116
> > +                a11y_registryd = Executive().popen(["%s/at-spi2-registryd" % atspi2_exec_path],
> > +                                                   env=test_env, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
> 
> Ditto.

Fixed. In this case I kept using popen() because I need to make sure I kill the process later on.

Also, I fixed another bug in there: 

   Bad:
     Executive().popen(["%s/at-spi2-registryd" % atspi2_exec_path] ...

  Good:
     Executive().popen([a11y_registryd_path] ...

Last, I added another modification to this patch, to avoid the following kind of errors when generating the docs:

  Generating WebKit2 documentation...
  ./webkit2gtk-unused.txt:1: warning: 11 unused declarations.They should be added to webkit2gtk-sections.txt in the appropriate place.
  ./webkit2gtk-unused.txt:1: warning: 11 unused declarations.They should be added to webkit2gtk-sections.txt in the appropriate place.

   gtkdoc did not build without warnings

The modification is as follows:

diff --git a/Tools/gtk/generate-gtkdoc b/Tools/gtk/generate-gtkdoc
index db5f83a..0c18a38 100755
--- a/Tools/gtk/generate-gtkdoc
+++ b/Tools/gtk/generate-gtkdoc
@@ -62,6 +62,7 @@ def get_webkit2_options():
                          glob.glob(src_path('PageClientImpl.*')) + \
                          glob.glob(src_path('WebKitUIClient.*')) + \
                          glob.glob(src_path('WebKitWebLoaderClient.*')) + \
+                         glob.glob(src_path('WebKitWebViewBaseAccessible.*')) + \
                          glob.glob(src_path('tests/*.h'))
     })
     return (common.build_path('Source', 'WebKit2', 'webkit2gtk-3.0.pc'), options)


That's all I think. Thanks!

[Carlos Garcia Campos]

Comment on attachment 121814 [details]
Patch proposal

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

Looks good to me in general, great work!

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:88
> +    AtkObject* rootObject = accessibilityRootObjectWrapper(atkObject);
> +    if (!rootObject)
> +        return 0;
> +
> +    return 1;

This could be simply:

return accessibilityRootObjectWrapper(atkObject) ? 1 : 0;

[Mario Sanchez Prada]

(In reply to comment #31)
> (From update of attachment 121814 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121814&action=review
> 
> Looks good to me in general, great work!

Thanks again, Carlos. Now it would be great if just one reviewer could perform more formal review of the patch (/me grins) :-)

> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:88
> > +    AtkObject* rootObject = accessibilityRootObjectWrapper(atkObject);
> > +    if (!rootObject)
> > +        return 0;
> > +
> > +    return 1;
> 
> This could be simply:
> 
> return accessibilityRootObjectWrapper(atkObject) ? 1 : 0;

Will keep this comment in mind for the next round of "changes", that I'm sure will happen after the patch starts getting formal reviews

[Martin Robinson]

Comment on attachment 121814 [details]
Patch proposal

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

This look fabulous! I have a bunch of really minor comments below and one or two larger concerns.

> Source/WebKit2/ChangeLog:9
> +        architecture of WK2 through AtkSocket ad AtkPlug.

ad -> and

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:2
> + * Copyright (C) 2011 Igalia S.L.

This should be 2012 now.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:12
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *

Let's use LPGL here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:7
> + * Copyright (C) 2011 Igalia S.L.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright

2012 and LGPL again.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:30
> +#include <gtk/gtk.h>

I don't think you need the GTK+ include here.

> Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:2
> + * Copyright (C) 2011 Igalia S.L.

2012

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:2
> + * Copyright (C) 2011 Igalia S.L.

2012

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:28
> +// The libatspi headers don't use G_BEGIN_DECLS
> +extern "C" {
> +#include <atspi/atspi.h>
> +}

Drop this down to the last include if possible to avoid breaking the block of includes.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:524
> +#if PLATFORM(GTK)
> +    // Ensure the accessibility hierarchy is updated after loading.
> +    webPage->updateAccessibilityTree();
> +#endif

Are you trying to find the right time to hook into the DOM? If so this should be in ::dispatchDidClearWindowObjectInWorld, I believe.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:1
> +/*

These files should be called Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObject.cpp/.h because they aren't GTK+ specific implementation files of cross-platform classes.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:2
> + * Copyright (C) 2011 Igalia S.L.

2012 here.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:94
> +    if (index < 0 || index > 1)

index != 0 && index != 1?

> Tools/Scripts/run-gtk-tests:67
> +    def _lookupAtspi2Binary(self, jhbuild_path, filename):

I'm pretty sure PEP-8 naming conventions say this should be _lookup_atspi2_binary or _find_atspi2_binary, but I'm not certain here.

> Tools/Scripts/run-gtk-tests:95
> +        # Make sure there's no X's property AT_SPI_BUS previously set,

X's property AT_SPI_BUS -> AT_SPI_BUS X property?

[Martin Robinson]

Mario asked me to clarify the licensing situation. In general, in WebCore/WebKit we try to match the licenses of other files in the directories. In the API though we are shooting for 100% LGPL.

[Mario Sanchez Prada]

Created attachment 123143 [details]
Patch Proposal

(In reply to comment #33)
> (From update of attachment 121814 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121814&action=review
> 
> This look fabulous! I have a bunch of really minor comments below and one or two larger concerns.

Thanks! I think the new patch addresses all the issus you pointed out. Just making a couple of minor comments on some of them. Find them below...

> > Source/WebKit2/ChangeLog:9
> > +        architecture of WK2 through AtkSocket ad AtkPlug.
> 
> ad -> and

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:2
> > + * Copyright (C) 2011 Igalia S.L.
> 
> This should be 2012 now.

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:12
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + * 1. Redistributions of source code must retain the above copyright
> > + *    notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *    notice, this list of conditions and the following disclaimer in the
> > + *    documentation and/or other materials provided with the distribution.
> > + *
> 
> Let's use LPGL here.

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:7
> > + * Copyright (C) 2011 Igalia S.L.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + * 1. Redistributions of source code must retain the above copyright
> 
> 2012 and LGPL again.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:30
> > +#include <gtk/gtk.h>
> 
> I don't think you need the GTK+ include here.

Fixed (moved to the implementation cpp file)

> > Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:2
> > + * Copyright (C) 2011 Igalia S.L.
> 
> 2012

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:2
> > + * Copyright (C) 2011 Igalia S.L.
> 
> 2012

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:28
> > +// The libatspi headers don't use G_BEGIN_DECLS
> > +extern "C" {
> > +#include <atspi/atspi.h>
> > +}
> 
> Drop this down to the last include if possible to avoid breaking the block of includes.

Hmm... can't do this without getting check-webkit-style complaining about unsorted list of includes, so I left it at the beginning of the list of global (system) includes

> > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:524
> > +#if PLATFORM(GTK)
> > +    // Ensure the accessibility hierarchy is updated after loading.
> > +    webPage->updateAccessibilityTree();
> > +#endif
> 
> Are you trying to find the right time to hook into the DOM? If so this should be in ::dispatchDidClearWindowObjectInWorld, I believe.

You're right. Thanks for pointing it out. Fixed.

> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:1
> > +/*
> 
> These files should be called Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObject.cpp/.h because they aren't GTK+ specific implementation files of cross-platform classes.

Ok. Fixed. Updated callers.

> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:2
> > + * Copyright (C) 2011 Igalia S.L.
> 
> 2012 here.

Fixed.

> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:94
> > +    if (index < 0 || index > 1)
> 
> index != 0 && index != 1?

Fixed.

> > Tools/Scripts/run-gtk-tests:67
> > +    def _lookupAtspi2Binary(self, jhbuild_path, filename):
> 
> I'm pretty sure PEP-8 naming conventions say this should be _lookup_atspi2_binary or _find_atspi2_binary, but I'm not certain here.

Fixed (used _lookup_atspi2_binary).

> > Tools/Scripts/run-gtk-tests:95
> > +        # Make sure there's no X's property AT_SPI_BUS previously set,
> 
> X's property AT_SPI_BUS -> AT_SPI_BUS X property?

Fixed.

[Martin Robinson]

Comment on attachment 123143 [details]
Patch Proposal

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

Looks good, but please make the following change before landing.

> Source/WebKit2/UIProcess/WebPageProxy.h:575
> +    String getAccessibilityPlugID() const { return m_accessibilityPlugID; }

In WebKit we typically don't use the getFoo naming scheme. Instead we just use foo().

[Mario Sanchez Prada]

Committed r105503: <http://trac.webkit.org/changeset/105503>