> 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).
> 
fixed

> 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).
>
done
 
> 
> 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>
> 
in the next branch I will remove that temporary code, with real code to return the indicators. So just added this so that it shows something in accerciser.

> 
> 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.
>
right, fixed

> 
> 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).
> 
ok, fixed

> 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;
>
yes, seems I got the format header wrong, so I've removed it from all the files and fixed (hopefully) all TABs
 
> 
>   * 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.
>
ok, file a bug for that and assign it to me and I'll work on that on another branch