[Webkit-unassigned] [Bug 72589] [GTK] Expose accessibility hierarchy in WebKit2 to ATK/AT-SPI based ATs
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Dec 6 03:29:15 PST 2011
https://bugs.webkit.org/show_bug.cgi?id=72589
--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> 2011-12-06 03:29:15 PST ---
(From update of attachment 117799)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list