Code review comment for lp:~rodrigo-moya/unity/add-panel-service-a11y

154 + if (!a11y_initialized)
155 + {

I think that it is worth to check the opposite, return if a11y_initialized. That would avoid a indentation level (IMHO, code would be clearer).

164 + /* Restore environment to load AT bridge */
165 + g_unsetenv ("NO_AT_BRIDGE");
166 + g_unsetenv ("NO_GAIL");

Take into account that there are one case (!should_enable_a11y) where you don't restore the environment, *but*, you modify the environment alway. Sorry, I was the one that suggested to move that to just before the load bridge, but probably this should be done as the first thing, to ensure that you always restore the environment (although this would only be a problem if there are a process child of this service, unlikely).

299 + atk_object_set_name (accessible, _("An indicator")); /* FIXME */

Do you really thing that worths to add this temporal names. Are useful for testing during development, but not sure if it is worth on have that on the actual code. If in the end you remove it, you will not require this include:
240 +#include <glib/gi18n.h>

294 +static void
295 +panel_indicator_accessible_initialize (AtkObject *accessible, gpointer data)
296 +{
297 + g_return_if_fail (PANEL_IS_INDICATOR_ACCESSIBLE (accessible));
298 +
299 + atk_object_set_name (accessible, _("An indicator")); /* FIXME */
300 + atk_object_set_role (accessible, ATK_ROLE_LABEL);
301 +
302 + ATK_OBJECT_CLASS (panel_indicator_accessible_parent_class)->initialize (accessible, data);

Although cally-root also calls the parent class initialize as the last thing, by default this should be done as the first thing of the initialize. Ie: imagine that the parent class also set a role, in that case that ATK_ROLE_LABEL would be unused.

508 + accessible->role = ATK_ROLE_APPLICATION;
509 + atk_object_set_name (accessible, _("Unity Panel Service"));

As the root represents the application I think that you should use the application name. Here you can use g_get_prgname(), like in gail or cally. I couldn't use that on Unity because I didn't found a equivalent to g_get_prgname to be used there (anyway, this is FIXME).

Some extra comments:

  * The indentation is somewhat weird. It seems that in some cases you have spaces and in other you have tabs, ie:
475 + atk_class->initialize = panel_root_accessible_initialize;
476 + atk_class->get_n_children = panel_root_accessible_get_n_children;
477 + atk_class->ref_child = panel_root_accessible_ref_child;
478 + atk_class->get_parent = panel_root_accessible_get_parent;

  * The way we reuse gail here is allow gtk_init to load gail but not the bridge, and then load the bridge. I don't like too much this mixed approach, as it could be prone-error (what would happen if for any reason gtk_init doesn't load the accessibility but panel-service yes, or the opposite). Taking into account the current wisdom (firefox and so on), probably we should do all the work. That means setting both NO_GAIL and NO_AT_BRIDGE, and then load by hand both gail and the bridge. But this could be done in a posterior task, but take that into account if you want to do that while solving the other issues.