Comment 45 for bug 857153

Comment on attachment 581750
Updated patch.

Thank you for tidying this up and using the async API to do this in parallel.
This approach looks good to me.

I wonder whether the PreInit() in ShouldA11yBeEnabled() is really necessary or
could be replaced with an assertion to check that PreInit had already been
called. Perhaps it is needed for unit tests?

>+static DBusPendingCall *a11yPendingCall = NULL;

I'm not familiar with the naming conventions in this particular code, but
usually static variables in Gecko have an "s" prefix.

>+ DBusError error;
>+ dbus_error_init(&error);
>+ DBusConnection* bus = dbus_bus_get(DBUS_BUS_SESSION, &error);
>+ if (!bus)
>+ return;

error is not actually used here, and so can be replaced with NULL.
"By convention, all functions allow NULL instead of a DBusError*, so callers who don't care about the error can ignore it."

>+ dbus_connection_send_with_reply(bus, message, &a11yPendingCall, 1000);
>+
>+dbus_done:
>+ if (message)
>+ dbus_message_unref(message);

I would move the dbus_message_unref to immediately after the send, before
dbus_done, so that the "if (message)" check is not required.

>+ if (bus)
>+ dbus_connection_unref(bus);

bus is always non-NULL here, so no need for the "if (bus)" check.

>+ strcmp(dbus_message_get_signature (reply), "v"))

Use DBUS_TYPE_VARIANT_AS_STRING instead of "v".

>+ dbus_done:
>+ if (reply)

Usually labels are "out"dented to make them stand out.

>- if (aState && sAccessibilityEnabled) {
>+ if (aState && a11y::ShouldA11yBeEnabled())
> CreateRootAccessible();
>- }

Please don't unbrace the block here. Convention in this file is heading
towards always brace except for jump statements, but usually leave existing code as is.

What does CreateRootAccessible() actually achieve? It looks like
mRootAccessible is unused. Is it holding a reference so that
DispatchEventToRootAccessible will somehow not need to create another
nsAccessible?

(In reply to Trevor Saunders (:tbsaunde) from comment #36)
> Comment on attachment 581750

> >+ dbusSuccess = true;
> >+ }
> >+
> >+ break;
>
> nit, put break in block above.

The "break" needs to be out of the block so as not to fall through to the DBUS_TYPE_BOOLEAN case when the type doesn't match, but the blank line there looks a bit odd to me.