[Bug 857153] Re: Needs to get accessibility settings from GSettings
Bug Watch Updater
857153 at bugs.launchpad.net
Sun Jan 1 05:14:52 UTC 2012
Launchpad has imported 43 comments from the remote bug at
https://bugzilla.mozilla.org/show_bug.cgi?id=693343.
If you reply to an imported comment from within Launchpad, your comment
will be sent to the remote bug automatically. Read more about
Launchpad's inter-bugtracker facilities at
https://help.launchpad.net/InterBugTracking.
------------------------------------------------------------------------
On 2011-10-10T17:20:25+00:00 Mgorse wrote:
Currently, Firefox determines that accessibility is enabled if
GNOME_ACCESSIBILITY is set to 1 in the environment or if
/desktop/gnome/interface/accessibility is set to True in gconf. The
latter check successfully determines whether accessibility is enabled
for GNOME 2 but not for GNOME 3. Currently, a distribution would need
to, ie, patch the firefox loader script to set GNOME_ACCESSIBILITY=1 for
Firefox to be accessible under GNOME 3.
There is now a gsettings key (toolkit-accessibility in the
org.gnome.desktop.interface schema), but this is specific to GNOME, and
so using it may not be optimal with AT-SPI becoming usable in XFCE and
eventually KDE. In order to handle this, the AT-SPI bus launcher
(org.a11y.Bus) has a org.a11y.Status.IsEnabled property on the bus
object (/org/a11y/bus). Under GNOME, this is a proxy for the GSettings
key. Xfce will likely have code in the future to ensure that this
property is set correctly. I would recommend that Firefox should check
this property first, falling back to gconf if there is an error checking
the property, but I'm not sure how best to handle this in the code.
Currently, there is a system pref
(config.use_system_prefs.accessibility) which maps to the gconf key.
There is also code in several places that checks the value of
GNOME_ACCESSIBILITY. So I guess we want to somehow have custom handling
for this system prefernce, if that is doable, or write a function
somewhere to test whether a11y is enabled (making the D-Bus call and
falling back to checking the system pref on error) and use that in the
places where it is needed to check the state of accessibility.
Advice welcome.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/2
------------------------------------------------------------------------
On 2011-10-21T20:12:13+00:00 Mgorse wrote:
Created attachment 568753
Proposed patch.
Check org.a11y.Status.IsEnabled to determine whether accessibility is enabled
before checking gconf.
This is adding more code that is in two places; I don't know if there is
now a better way to handle it (see bug 390761).
An alternate approach would be to have mozilla.sh call dbus-send and set
GNOME_ACCESSIBILITY=1 if the dbus property is set to true and
GNOME_ACCESSIBILITY is unset.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/3
------------------------------------------------------------------------
On 2011-10-25T05:05:39+00:00 Ginn-chen-r wrote:
I don't know how much time dbus will take. I hope it will not slow down
startup time.
Since libxul is always enabled now, i.e. nsWindows.cpp and
nsApplicationAccessibleWrap.cpp are linked into libxul.so, I think you
may not need to copy the code twice.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/4
------------------------------------------------------------------------
On 2011-10-25T12:02:21+00:00 Trevor Saunders wrote:
(In reply to Ginn Chen from comment #2)
> I don't know how much time dbus will take. I hope it will not slow down
> startup time.
I'm a little concerned about this too, but I suspect doing the dbus call
off the main thread will be enough work that we should get real numbers
before deciding to do it.
> Since libxul is always enabled now, i.e. nsWindows.cpp and
> nsApplicationAccessibleWrap.cpp are linked into libxul.so, I think you may
> not need to copy the code twice.
I suspect this is correct, but I'm ok with not cleaning up the existing
dupplicated code in this bug. What I'd suggest is put the function to
check the dbus status in the a11y namespace and make it non-static and
use in the widget check.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/5
------------------------------------------------------------------------
On 2011-10-25T12:15:00+00:00 Trevor Saunders wrote:
Comment on attachment 568753
Proposed patch.
>+static bool
>+test_a11y_dbus (bool *out)
I think I'd use IsDBusA11yEnabled()
>+{
>+ // XXX following code is copied from widget/src/gtk2/nsWindow.cpp
>+ // we should put it somewhere that can be used from both modules
>+ // see bug 390761
>+ bool retval = FALSE;
>+#ifdef MOZ_ENABLE_DBUS
>+ DBusConnection *bus;
>+ DBusMessage *message = NULL, *reply = NULL;
>+ DBusMessageIter iter, iter_variant, iter_struct;
>+ dbus_bool_t d_result;
>+ DBusError error;
this isn't ansi c89 ;) please declare these closer to where they are used.
>+ reply = dbus_connection_send_with_reply_and_block(bus, message, 1000, &error);
>+ if (!reply ||
>+ dbus_message_get_type (reply) != DBUS_MESSAGE_TYPE_METHOD_RETURN ||
>+ strcmp (dbus_message_get_signature (reply), "v") != 0)
>+ goto exit;
blank line
btw what is "v" as a dbus signature?
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/6
------------------------------------------------------------------------
On 2011-10-25T18:38:49+00:00 Roc-ocallahan wrote:
Comment on attachment 568753
Proposed patch.
Review of attachment 568753:
-----------------------------------------------------------------
::: accessible/src/atk/nsApplicationAccessibleWrap.cpp
@@ +618,5 @@
> +test_a11y_dbus (bool *out)
> +{
> + // XXX following code is copied from widget/src/gtk2/nsWindow.cpp
> + // we should put it somewhere that can be used from both modules
> + // see bug 390761
Why not fix this now? You can just make the widget version nonstatic and
call it from here.
@@ +625,5 @@
> + DBusConnection *bus;
> + DBusMessage *message = NULL, *reply = NULL;
> + DBusMessageIter iter, iter_variant, iter_struct;
> + dbus_bool_t d_result;
> + DBusError error;
This is C++ code, just declare these where they're first assigned
wherever possible
@@ +627,5 @@
> + DBusMessageIter iter, iter_variant, iter_struct;
> + dbus_bool_t d_result;
> + DBusError error;
> + const char *iface = "org.a11y.Status";
> + const char *member = "IsEnabled";
static const char iface[] = ...;
static const char member[] = ...;
@@ +636,5 @@
> + goto exit;
> +
> + message = dbus_message_new_method_call ("org.a11y.Bus", "/org/a11y/bus",
> + "org.freedesktop.DBus.Properties",
> + "Get");
How fast is this? We're calling this on every widget creation, could
this be slow?
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/7
------------------------------------------------------------------------
On 2011-10-25T20:32:31+00:00 Trevor Saunders wrote:
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Comment on attachment 568753 [diff] [details] [review]
> Proposed patch.
>
> Review of attachment 568753 [diff] [details] [review]:
> -----------------------------------------------------------------
>
> ::: accessible/src/atk/nsApplicationAccessibleWrap.cpp
> @@ +618,5 @@
> > +test_a11y_dbus (bool *out)
> > +{
> > + // XXX following code is copied from widget/src/gtk2/nsWindow.cpp
> > + // we should put it somewhere that can be used from both modules
> > + // see bug 390761
>
> Why not fix this now? You can just make the widget version nonstatic and
> call it from here.
>
> @@ +625,5 @@
> > + DBusConnection *bus;
> > + DBusMessage *message = NULL, *reply = NULL;
> > + DBusMessageIter iter, iter_variant, iter_struct;
> > + dbus_bool_t d_result;
> > + DBusError error;
>
> This is C++ code, just declare these where they're first assigned wherever
> possible
>
> @@ +627,5 @@
> > + DBusMessageIter iter, iter_variant, iter_struct;
> > + dbus_bool_t d_result;
> > + DBusError error;
> > + const char *iface = "org.a11y.Status";
> > + const char *member = "IsEnabled";
>
> static const char iface[] = ...;
> static const char member[] = ...;
>
> @@ +636,5 @@
> > + goto exit;
> > +
> > + message = dbus_message_new_method_call ("org.a11y.Bus", "/org/a11y/bus",
> > + "org.freedesktop.DBus.Properties",
> > + "Get");
>
> How fast is this? We're calling this on every widget creation, could this be
> slow?
oh? it probably isn't the fastest thing in the world, but I'm not really
sure. If that code runs every time we create a widget (which I assume
we do a lot) we should probably get rid of the gconf check there too.
I'm not sure how fast gconf is, but I can't see it being terriffic.
The really correct solution here would be to call the dbus method once
on startup, and then ask dbus to send us a signal when it changes, but
I for one have no idea how easy that will be to do (I don't know
anything about how gecko currently interacts with dbus)
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/8
------------------------------------------------------------------------
On 2011-10-27T00:35:11+00:00 Mgorse wrote:
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
#5)
> > + const char *iface = "org.a11y.Status";
> > + const char *member = "IsEnabled";
>
> static const char iface[] = ...;
> static const char member[] = ...;
If I do this, then I get a seg fault, whether I preface the reference
with & or not. I need a char ** to pass to dbus_message_append_args.
> @@ +636,5 @@
> > + goto exit;
> > +
> > + message = dbus_message_new_method_call ("org.a11y.Bus", "/org/a11y/bus",
> > + "org.freedesktop.DBus.Properties",
> > + "Get");
>
> How fast is this? We're calling this on every widget creation, could this be
> slow?
On my laptop (2.4ghz Core 2 Duo), it takes 0.6ms for the call plus an
additional millisecond the first time a connection to the session bus is
established. I presume it would take longer on a slower machine. The
current code in widget/src/gtk2 records the result in a static variable,
so there it would only make the call once, but, regardless, I agree that
it is better to only have the code in one place if possible, so I'm
testing with the duplicate code removed.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/9
------------------------------------------------------------------------
On 2011-10-27T03:56:31+00:00 Mgorse wrote:
Created attachment 569898
Revised patch.
Remove duplicate code; add method in nsIAccessibilityService to test whether
a11y is enabled.
Also, unref the dbus connection when we're done with it.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/10
------------------------------------------------------------------------
On 2011-11-07T09:31:35+00:00 Trevor Saunders wrote:
Created attachment 572408
patch
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/11
------------------------------------------------------------------------
On 2011-11-07T15:23:05+00:00 Bolterbugz wrote:
Comment on attachment 572408
patch
Review of attachment 572408:
-----------------------------------------------------------------
Please don't forget to add mgorse as an author if you've built on his
work.
::: accessible/src/base/nsAccessibilityService.cpp
@@ +1845,5 @@
> }
> +
> +#ifdef MOZ_ACCESSIBILITY_ATK
> +bool
> +ShouldA11yBeEnabled()
Would probably be better to move this into our atk layer no?
::: accessible/src/base/nsAccessibilityService.h
@@ +58,5 @@
>
> +/**
> + * Is platform accessibility enabled.
> + */
> +bool ShouldA11yBeEnabled();
Despite the name is Linux/dbus specific right?
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/12
------------------------------------------------------------------------
On 2011-11-07T15:53:19+00:00 Bolterbugz wrote:
Try server doesn't seem to have debus?
../../../dist/system_wrappers/dbus/dbus.h:3:28: fatal error: dbus/dbus.h: No such file or directory
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/13
------------------------------------------------------------------------
On 2011-11-07T18:39:00+00:00 Trevor Saunders wrote:
Created attachment 572525
patch
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/14
------------------------------------------------------------------------
On 2011-11-07T21:03:25+00:00 Hub-g wrote:
(In reply to Trevor Saunders (:tbsaunde) from comment #12)
> Created attachment 572525 [diff] [details] [review]
> patch
I can't compile with that patch. Can't seem to find the dbus/dbus.h
header. There is probably a magic trick I forgot about.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/15
------------------------------------------------------------------------
On 2011-11-07T21:26:44+00:00 Hub-g wrote:
Comment on attachment 572525
patch
Review of attachment 572525:
-----------------------------------------------------------------
With this patch I can't compile.
we need to add access to the dbus headers in
accessible/src/base/Makefile.in
::: accessible/src/base/nsAccessibilityService.cpp
@@ +1837,5 @@
> ////////////////////////////////////////////////////////////////////////////////
>
> +namespace mozilla {
> +namespace a11y {
> + mozilla::a11y::FocusManager*
We should remove the namespace qualifier here since we are inside it.
@@ +1842,1 @@
> mozilla::a11y::FocusMgr()
and here
@@ +1842,5 @@
> mozilla::a11y::FocusMgr()
> {
> return nsAccessibilityService::gAccessibilityService;
> }
> +
We are missing
}
}
to close the namespaces above.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/16
------------------------------------------------------------------------
On 2011-11-07T21:48:15+00:00 Trevor Saunders wrote:
Created attachment 572602
PATCH
NO IDEA WHAT WENT WRONG WITH THE LAST PATCH, i WAS SURE IT COMPILED
LOCALLY, BUT THAT MAKES NO SENSE, i JUST HAVE NO IDEA.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/17
------------------------------------------------------------------------
On 2011-11-07T21:55:03+00:00 Trevor Saunders wrote:
Comment on attachment 572602
PATCH
>+
>diff --git a/accessible/src/base/nsAccessibilityService.cpp b/accessible/src/base/nsAccessibilityService.cpp
>--- a/accessible/src/base/nsAccessibilityService.cpp
>+++ b/accessible/src/base/nsAccessibilityService.cpp
>@@ -81,6 +81,7 @@
> #include "nsTextFragment.h"
> #include "mozilla/Services.h"
> #include "nsEventStates.h"
>+#include "prenv.h"
artifact will remove before landing
> #ifdef MOZ_XUL
> #include "nsXULAlertAccessible.h"
>@@ -1832,8 +1833,9 @@
> // Services
> ////////////////////////////////////////////////////////////////////////////////
>
>-mozilla::a11y::FocusManager*
>+ mozilla::a11y::FocusManager*
> mozilla::a11y::FocusMgr()
ugh, this not my day :(
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/18
------------------------------------------------------------------------
On 2011-11-07T23:12:26+00:00 Hub-g wrote:
Comment on attachment 572602
PATCH
Review of attachment 572602:
-----------------------------------------------------------------
::: accessible/src/atk/nsApplicationAccessibleWrap.cpp
@@ +912,5 @@
> + switch (dbus_message_iter_get_arg_type (&iter_variant)) {
> + case DBUS_TYPE_STRUCT:
> + // at-spi2-core 2.2.0-2.2.1 had a bug where it returned a struct
> + dbus_message_iter_recurse (&iter_variant, &iter_struct);
> + if (dbus_message_iter_get_arg_type (&iter_struct) != DBUS_TYPE_BOOLEAN) {
Did you mean == DBUS_TYPE_BOOLEAN instead?
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/19
------------------------------------------------------------------------
On 2011-11-07T23:39:08+00:00 Hub-g wrote:
Comment on attachment 572602
PATCH
Review of attachment 572602:
-----------------------------------------------------------------
::: accessible/src/base/nsAccessibilityService.cpp
@@ +81,4 @@
> #include "nsTextFragment.h"
> #include "mozilla/Services.h"
> #include "nsEventStates.h"
> +#include "prenv.h"
is this needed?
@@ +1833,4 @@
> // Services
> ////////////////////////////////////////////////////////////////////////////////
>
> + mozilla::a11y::FocusManager*
I don't see the need for the indentation. just nitpicking here.
::: widget/src/gtk2/nsWindow.cpp
@@ +1110,4 @@
> }
>
> #ifdef ACCESSIBILITY
> + if (aState && a11y::ShouldA11yBeEnabled())
in nsAccessibilityService.h, ShouldA11yBeEnabled() is declared only we
use ATK, while here it seems to be for the ACESSIBILITY.
@@ +6475,4 @@
> void
> nsWindow::DispatchEventToRootAccessible(PRUint32 aEventType)
> {
> + if (!a11y::ShouldA11yBeEnabled())
same as above.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/20
------------------------------------------------------------------------
On 2011-11-07T23:49:12+00:00 Trevor Saunders wrote:
(In reply to Hub Figuiere [:hub] from comment #18)
> Comment on attachment 572602 [diff] [details] [review]
> PATCH
>
> Review of attachment 572602 [diff] [details] [review]:
> -----------------------------------------------------------------
>
> ::: accessible/src/base/nsAccessibilityService.cpp
> @@ +81,4 @@
> > #include "nsTextFragment.h"
> > #include "mozilla/Services.h"
> > #include "nsEventStates.h"
> > +#include "prenv.h"
>
> is this needed?
>
> @@ +1833,4 @@
> > // Services
> > ////////////////////////////////////////////////////////////////////////////////
> >
> > + mozilla::a11y::FocusManager*
>
> I don't see the need for the indentation. just nitpicking here.
yeah, see my earlier comment, these two are fixed locally, I can upload
a new patch if you like.
> ::: widget/src/gtk2/nsWindow.cpp
> @@ +1110,4 @@
> > }
> >
> > #ifdef ACCESSIBILITY
> > + if (aState && a11y::ShouldA11yBeEnabled())
>
> in nsAccessibilityService.h, ShouldA11yBeEnabled() is declared only we use
> ATK, while here it seems to be for the ACESSIBILITY.
I guess saying its only for atk is a little misleading, its really for
desktop linux where we use atk / at-spi
However there is a sort of implicit assumption that the gtk backend will
be related to using atk accessibility (which is sort of reasonable since
gtk uses atk internally).
Another thing is that for atk we need general accessibility to suport
atk.
I'd be willing to improve the comment if you have ideas.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/21
------------------------------------------------------------------------
On 2011-11-07T23:53:31+00:00 Roc-ocallahan wrote:
Comment on attachment 572602
PATCH
Review of attachment 572602:
-----------------------------------------------------------------
More dbus usage ...
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/22
------------------------------------------------------------------------
On 2011-11-08T00:02:32+00:00 Trevor Saunders wrote:
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Comment on attachment 572602 [diff] [details] [review]
> PATCH
>
> Review of attachment 572602 [diff] [details] [review]:
> -----------------------------------------------------------------
>
> More dbus usage ...
true... I'm not saying I like it, but I think we need to get used to
linux a11y using it. We haven't really had to deal with it since its
all been hidden behind atk so far, but at-spi2 is all dbus and people
have been using firefox a11y with at-spi2 for a while now, and we will
need to drop support for at-spi over corba for e10s because of atk plug
/ socket only being available in at-spi2. A fall back of multiple
accessible trees has been discused, but I'm not particularly interested
personally. Finally whatever we think of it I doubt there's much we can
do to change that at-spi2 is the future.
All that said other ideas welcome!
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/23
------------------------------------------------------------------------
On 2011-11-08T00:21:52+00:00 Hub-g wrote:
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Comment on attachment 572602 [diff] [details] [review]
> PATCH
>
> Review of attachment 572602 [diff] [details] [review]:
> -----------------------------------------------------------------
>
> More dbus usage ...
To me it looks to be the saner way to do it without mandating Gnome3.
Gsettings being worse.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/24
------------------------------------------------------------------------
On 2011-11-08T01:57:22+00:00 Trevor Saunders wrote:
Created attachment 572700
patch with nits fixed
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/25
------------------------------------------------------------------------
On 2011-11-08T02:43:37+00:00 Karlt wrote:
(In reply to Trevor Saunders (:tbsaunde) from comment #21)
(In reply to Hub Figuiere [:hub] from comment #22)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> > More dbus usage ...
I think roc was talking to me with this comment.
DBus definitely sounds preferable to GSettings.
(And DBus sounds like the right way to do GNOME 3 desktop settings stored with GSettings.)
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/26
------------------------------------------------------------------------
On 2011-11-08T03:33:25+00:00 Trevor Saunders wrote:
(In reply to Karl Tomlinson (:karlt) from comment #24)
> (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> (In reply to Hub Figuiere [:hub] from comment #22)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
>
> > > More dbus usage ...
>
> I think roc was talking to me with this comment.
>
> DBus definitely sounds preferable to GSettings.
> (And DBus sounds like the right way to do GNOME 3 desktop settings stored
> with GSettings.)
Oh, Ok, ftr I'm pretty confident in the dbus code at this point, I haven't really changed it much just try and make it more gecko styley and it looks fine to me. I beleive Hub has dealt with dbus some it seems like he thinks its reasonable too. Finally at-spi2 and the atk-bridge for dbus is basically Mike's at this point so I generally trust the dbus code he writes.
any chance you can review this soon? It's a bit of a usability problem since currently as the bug says firefox appears to be inaccessible in modern linux installs.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/27
------------------------------------------------------------------------
On 2011-11-08T06:01:41+00:00 Karlt wrote:
Comment on attachment 572700
patch with nits fixed
Can you provide function names and 8 lines of context with future patches,
please? https://developer.mozilla.org/en/Installing_Mercurial#Configuration
>+ DBusConnection* bus = dbus_bus_get(DBUS_BUS_SESSION, &error);
dbus_bus_get() will sometimes call
dbus_connection_set_exit_on_disconnect(TRUE), so that needs to be undone with
another dbus_connection_set_exit_on_disconnect().
>+ reply = dbus_connection_send_with_reply_and_block(bus, message,
1000, &error);
We are actively trying to shorten the start-up path to reduce start-up time.
Blocking on DBus during creation of the first window wouldn't be helpful.
Is it feasible to initialize on receipt of an async reply?
Or I expect it would be possible to create a DBusPendingCall on window
creation, and then only block on it and steal_reply when it is really needed?
(In reply to Trevor Saunders (:tbsaunde) from comment #25)
> It's a bit of a usability problem
> since currently as the bug says firefox appears to be inaccessible in modern
> linux installs.
I thought GNOME 3 distros were at least providing a read-only GConf
wrapper.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/28
------------------------------------------------------------------------
On 2011-11-08T10:13:14+00:00 Trevor Saunders wrote:
(In reply to Karl Tomlinson (:karlt) from comment #26)
> Comment on attachment 572700 [diff] [details] [review]
> patch with nits fixed
>
> Can you provide function names and 8 lines of context with future patches,
> please? https://developer.mozilla.org/en/Installing_Mercurial#Configuration
>
> >+ DBusConnection* bus = dbus_bus_get(DBUS_BUS_SESSION, &error);
>
> dbus_bus_get() will sometimes call
> dbus_connection_set_exit_on_disconnect(TRUE), so that needs to be undone with
> another dbus_connection_set_exit_on_disconnect().
... great
> >+ reply = dbus_connection_send_with_reply_and_block(bus, message, 1000, &error);
>
> We are actively trying to shorten the start-up path to reduce start-up time.
> Blocking on DBus during creation of the first window wouldn't be helpful.
>
> Is it feasible to initialize on receipt of an async reply?
I'm not sure how bad this is in practice, but I don't forsee any huge
problem with using pending calls to do this async.
> (In reply to Trevor Saunders (:tbsaunde) from comment #25)
> > It's a bit of a usability problem
> > since currently as the bug says firefox appears to be inaccessible in modern
> > linux installs.
>
> I thought GNOME 3 distros were at least providing a read-only GConf wrapper.
tbh I'm not sure, but I know we had several people confused about the
problem
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/29
------------------------------------------------------------------------
On 2011-11-14T18:31:30+00:00 Hub-g wrote:
Comment on attachment 572700
patch with nits fixed
Review of attachment 572700:
-----------------------------------------------------------------
This looks good to me.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/30
------------------------------------------------------------------------
On 2011-11-16T13:53:56+00:00 Alexander Surkov wrote:
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
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/31
------------------------------------------------------------------------
On 2011-11-17T04:40:41+00:00 Alexander Surkov wrote:
(In reply to alexander surkov from comment #29)
> > bool
> > nsApplicationAccessibleWrap::Init()
> > {
> > + if (ShouldA11yBeEnabled()) {
>
> if application accessible is initialized then accessibility must be enabled.
> What's a reason of this check?
I see you do that because of bug 390761.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/32
------------------------------------------------------------------------
On 2011-11-20T04:26:47+00:00 Mgorse wrote:
Created attachment 575724
patch #7
Add a PreInit() function to be called on window creation, that establishes a
D-Bus connection and makes a pending call to determine whether accessibility
is enabled. Have ShouldA11yBeEnabled block on this call if needed. Hopefully
this will reduce start-up time slightly. (In theory we could probably
only enable a11y on a response to the pending call, but this would be much
more of an architectural change, and we would also need to keep in mind that
currently we are still supporting gconf as a callback, so this approach
is a lot simpler.)
Also, call dbus_connection_set_exit_on_disconnect, and fix some nits.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/33
------------------------------------------------------------------------
On 2011-11-29T01:33:47+00:00 Mgorse wrote:
Created attachment 577444
Updated patch.
Update to apply against current source.
Add a couple of MOZ_A11Y_DBUS checks where they were missing.
Remove some includes that are no longer needed.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/34
------------------------------------------------------------------------
On 2011-11-29T03:31:00+00:00 Trevor Saunders wrote:
(In reply to Mike Gorse from comment #32)
> Created attachment 577444 [diff] [details] [review]
> Updated patch.
You didn't get the merge with bug 451161 right, you should probably
check the use system pref first then dbus or gconf.
I'm also thinking it might be nice to put this stuff in a new file, Alex
thoughts?
common nit, I tend to prefer
if (x)
return;
foo();
to
if (x)
return;
foo();
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/35
------------------------------------------------------------------------
On 2011-11-29T11:28:05+00:00 Alexander Surkov wrote:
(In reply to Trevor Saunders (:tbsaunde) from comment #33)
> I'm also thinking it might be nice to put this stuff in a new file, Alex
> thoughts?
new file might be nice but actually I'd like to figure out general
concept how we should handle initialization. I filed bug 706051 for
that.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/36
------------------------------------------------------------------------
On 2011-12-14T20:25:35+00:00 Mgorse wrote:
Created attachment 581750
Updated patch.
Merge with 451161/705983.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/37
------------------------------------------------------------------------
On 2011-12-15T03:33:02+00:00 Trevor Saunders wrote:
Comment on attachment 581750
Updated patch.
>+PreInit()
>+{
>+#ifdef MOZ_ENABLE_DBUS
>+ static bool sChecked = FALSE;
>+ if (sChecked)
>+ return;
>+ sChecked = TRUE;
nit, blank line before sChecked = true
I'm tempted to think we should bail if GNOME_ACCESSIBILITY is set since we should bail after this if GNOME_ACCESSIBILITY is set, otherwise nobody will ever check the return message.
>+ if (!bus)
>+ return;
>+ dbus_connection_set_exit_on_disconnect(bus, FALSE);
nit, blank line.
>+ if (a11yPendingCall) {
nit,if ( !a11yPendingCall) goto dbus_done;
also we don't usually put a11y in local variables.
>+ dbusSuccess = true;
>+ }
>+
>+ break;
nit, put break in block above.
>diff --git a/widget/src/gtk2/nsWindow.cpp b/widget/src/gtk2/nsWindow.cpp
> #ifdef ACCESSIBILITY
>-#include "nsIAccessibilityService.h"
>+#include "nsAccessibilityService.h"
> #include "nsIAccessibleDocument.h"
>-#include "prenv.h"
>-#include "stdlib.h"
>
> using namespace mozilla;
> using namespace mozilla::widget;
btw while I suppose ti doesn't really happen since these are present
later unconditionally why are these here?
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/38
------------------------------------------------------------------------
On 2011-12-15T03:48:50+00:00 Trevor Saunders wrote:
> @@ +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
This code is pretty "Cish" because of the dbus api, and its a
reasonably common view that gotos like this are good style for the
special case of error handling in C.
> ::: 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.
you mean nsAccessibilityService::GetAccService() is non-null right?
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/39
------------------------------------------------------------------------
On 2011-12-20T00:21:21+00:00 Karlt wrote:
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.
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/40
------------------------------------------------------------------------
On 2011-12-20T15:09:54+00:00 Trevor Saunders wrote:
landed
https://hg.mozilla.org/mozilla-central/rev/860fdd41cfed
https://hg.mozilla.org/mozilla-central/rev/887abec76bca
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/41
------------------------------------------------------------------------
On 2011-12-20T16:07:25+00:00 Bmo-edmorley wrote:
Backed out per #developers:
https://hg.mozilla.org/mozilla-central/rev/6af20fb62fb7
https://tbpl.mozilla.org/php/getParsedLog.php?id=8060865&tree=Firefox
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/42
------------------------------------------------------------------------
On 2011-12-30T03:19:25+00:00 Trevor Saunders wrote:
*** Bug 714198 has been marked as a duplicate of this bug. ***
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/48
------------------------------------------------------------------------
On 2011-12-30T03:48:48+00:00 Karlt wrote:
The tinderbox logs are no longer found at the link above.
If there is no DBus daemon already running, I've seen DBus spawn a child
process that doesn't properly disconnect from ssh sessions; I wonder
whether something similar might be happening here.
These IRC comments from
http://bugzilla.glob.com.au/irc/?c=developers&a=date&s=2011-12-20&e=2011-12-21&h=
may be helpful.
<Ms2ger philor, fwiw, tbsaunde says he was green on try earlier
<tbsaunde https://tbpl.mozilla.org/?tree=Try&rev=083fd66ad8f2 fwiw
* philor loads up the try log
<philor> not that I don't trust try to run the same things that m-c runs, but, well, I don't trust to try to run the same things
<philor> interesting: try still does a couple of rather foolish and pointless steps, that wind up creating a profile which isn't actually used, and that's apparently enough
<Ms2ger> philor, the first word I can think of to describe that is "Mozilla"
<philor> so my wild guess, having been awake for 20 minutes, is that the patch introduces a shutdown hang or at least a shutdown really-slow, and try survives it by being foolish
<philor> "firefox-bin -no-remote -profile /builds/slave/try-lnx64-dbg/build/obj-firefox/_leaktest/leakprofile/ http://localhost:8888/bloatcycle.html -P default"
<philor> no idea how you could be saved by that bit of foolishness, either
<philor> tbsaunde: in theory, build with ac_add_options --enable-
debug and ac_add_options --enable-trace-malloc, cd objdir/_leaktest/,
python leaktest.py, then python leaktest.py --trace-malloc malloc.log
--shutdown-leaks=sdleak.log should repro
<philor> tbsaunde: no idea who would give you a stack, since I
don't even know what's happening - leaktest.py starts up an http server,
runs the browser, according to the log the browser closed, according to
the exit code leaktest.py closed and closed happy, but when it tries to
run again, it's still running from before
Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/49
** Changed in: firefox
Status: Unknown => Confirmed
** Changed in: firefox
Importance: Unknown => High
--
You received this bug notification because you are a member of Mozilla
Bugs, which is subscribed to firefox in Ubuntu.
https://bugs.launchpad.net/bugs/857153
Title:
Needs to get accessibility settings from GSettings
To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/857153/+subscriptions
More information about the Ubuntu-mozillateam-bugs
mailing list