Comment 36 for bug 857153

Comment on attachment 572700
patch with nits fixed

Review of attachment 572700:
-----------------------------------------------------------------

I'm not looking into the platform specific logic since I bet you did that well, just integration part.
canceling review until comments are addressed

::: accessible/src/atk/nsApplicationAccessibleWrap.cpp
@@ +612,4 @@
> bool
> nsApplicationAccessibleWrap::Init()
> {
> + if (ShouldA11yBeEnabled()) {

if application accessible is initialized then accessibility must be enabled. What's a reason of this check?

@@ +862,5 @@
> return NS_OK;
> }
> +
> +namespace mozilla {
> + namespace a11y {

nit: no indent for a11y namespace

@@ +865,5 @@
> +namespace mozilla {
> + namespace a11y {
> +
> +bool
> +ShouldA11yBeEnabled()

that's weird something prototyped in nsAccessibilityService gets defined in nsApplicationAccessibleWrap. I can't think of better place but you should XXX comment in prototype I think

@@ +869,5 @@
> +ShouldA11yBeEnabled()
> +{
> + static bool sChecked = false, sShouldEnable = false;
> + if (sChecked)
> + return sShouldEnable;

that's ok but curious why wouldn't use enum instead?

@@ +928,5 @@
> + default:
> + break;
> + }
> +
> + dbus_done:

goto is unusual style and it's not appreciated in c++ world in general. I'm fine if you're sure to keep it

@@ +955,5 @@
> + do_GetService(sSysPrefService, &rv);
> + if (NS_SUCCEEDED(rv) && sysPrefService)
> + sysPrefService->GetBoolPref(sAccessibilityKey, &sShouldEnable);
> +
> + return sShouldEnable;

nit: wrong indentation

::: accessible/src/base/nsAccessibilityService.h
@@ +57,5 @@
> FocusManager* FocusMgr();
>
> +/**
> + * Is platform accessibility enabled.
> + * only used on linux with atk for now.

nit: o -> O

@@ +61,5 @@
> + * only used on linux with atk for now.
> + */
> +#ifdef MOZ_ACCESSIBILITY_ATK
> +bool ShouldA11yBeEnabled();
> +#endif

nit: put comment into #ifdef please

::: widget/src/gtk2/nsWindow.cpp
@@ +6479,1 @@
> return;

it appears nsWindow::Show() manages root accessible creation, so here you should check only if accessibility service instantiated.

and then you have unique consumer of ShouldA11yBeEnabled (nsWindow::Show), maybe it's worth to keep this method somewhere in gtk2 code