[webkit-reviews] review requested: [Bug 72589] [GTK] Expose accessibility hierarchy in WebKit2 to ATK/AT-SPI based ATs : [Attachment 120955] Patch proposal
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 3 08:56:28 PST 2012
Mario Sanchez Prada <msanchez at igalia.com> has asked for review:
Bug 72589: [GTK] Expose accessibility hierarchy in WebKit2 to ATK/AT-SPI based
ATs
https://bugs.webkit.org/show_bug.cgi?id=72589
Attachment 120955: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=120955&action=review
------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
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.
More information about the webkit-reviews
mailing list